Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable copy-clean-link-by-default for osx #17752

Merged
merged 1 commit into from
Mar 25, 2023
Merged

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Mar 24, 2023

Resolves brave/brave-browser#29303

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Check brave-copy-clean-link-by-default feature disabled by default for osx and enabled for other platforms

@spylogsster spylogsster requested a review from fmarier March 24, 2023 18:36
@spylogsster spylogsster self-assigned this Mar 24, 2023
@spylogsster spylogsster marked this pull request as ready for review March 24, 2023 18:37
@fmarier fmarier changed the title Disable Copy clean link for osx Disable copy-clean-link-by-default for osx Mar 24, 2023
@spylogsster spylogsster force-pushed the clean-link-osx-off branch 5 times, most recently from c3c120f to ce0ca10 Compare March 24, 2023 21:05
@spylogsster spylogsster enabled auto-merge (squash) March 24, 2023 22:14
@spylogsster spylogsster force-pushed the clean-link-osx-off branch 2 times, most recently from 5ab750b to fe7641c Compare March 25, 2023 08:01
@spylogsster spylogsster merged commit c1ea55b into master Mar 25, 2023
@spylogsster spylogsster deleted the clean-link-osx-off branch March 25, 2023 12:26
@github-actions github-actions bot added this to the 1.51.x - Nightly milestone Mar 25, 2023
@spylogsster spylogsster restored the clean-link-osx-off branch March 25, 2023 14:24
brave-builds added a commit that referenced this pull request Mar 25, 2023
@LaurenWags
Copy link
Member

Verified with

Brave | 1.51.64 Chromium: 112.0.5615.39 (Official Build) nightly (x86_64)
-- | --
Revision | a0e7b9718a92bcd1cf33b7c95316caff3fc20714-refs/branch-heads/5615@{#753}
OS | macOS Version 12.6.3 (Build 21G419)
macOS - PASSED

Confirmed with prior version (1.51.51 via #17673 (comment)), that default for brave://flags/#brave-copy-clean-link-by-default was enabled.

Using 1.51.64, confirmed default for brave://flags/#brave-copy-clean-link-by-default was disabled:

0

Case A: Ran through appropriate cases on https://fmarier.github.io/brave-testing/copy-clean-link.html to confirm functionality still works with the keyboard shortcut disabled. (spot check of "Copy clean link" functionality)

Case B: Ran through the below steps to confirm expected locations of "Copy clean link" option and confirm that keyboard shortcut (Cmd + C on macOS) is not used for "Copy clean link":

  1. Confirmed value of "Default" for brave://flags/#brave-copy-clean-link-by-default
  2. Visited https://dev-pages.bravesoftware.com/clean-urls/?brave_testing1=foo&brave_testing2=bar&brave_testing3=keep&&;b&d&utm_content=removethis&e=&f=g&=end
  3. Clicked the URL bar and confirmed "Copy clean link" available from macOS app menu and it does NOT have the cmd + C keyboard shortcut (⌘C) listed next to it, this is listed next to "Copy" as expected
  4. Right-clicked the URL and confirmed "Copy clean link" is in the context menu
  5. Confirmed "Copy clean link" is displayed in the share menu from URL bar
  6. Used "Cmd + C" while the URL bar is in focus to copy the URL
  7. Opened a new tab and pasted the copied URL
  8. Confirmed link pasted is the original URL with all parameters: https://dev-pages.bravesoftware.com/clean-urls/?brave_testing1=foo&brave_testing2=bar&brave_testing3=keep&&;b&d&utm_content=removethis&e=&f=g&=end
  9. Opened a new tab and visited https://twitter.com/LBC/status/1577628501364146176?s=20&t=fpVMBMSfgI_pNGQxxpV6bw
  10. Used "Cmd + C" while the URL bar is in focus to copy the URL
  11. Opened a new tab and pasted the copied link
  12. Confirmed link pasted is the original URL with all parameters https://twitter.com/LBC/status/1577628501364146176?s=20&t=fpVMBMSfgI_pNGQxxpV6bw
Step 0 Step 2 Step 3 Step 4 Step 7 Step 8 Step 11
0 2 3 4 7 8 11

Brave	1.51.64 Chromium: 112.0.5615.39 (Official Build) nightly (64-bit) 
Revision	a0e7b9718a92bcd1cf33b7c95316caff3fc20714-refs/branch-heads/5615@{#753}
OS	Windows 10 Version 21H2 (Build 19044.2486)
Windows - PASSED

Spot checked that the default for brave://flags/#brave-copy-clean-link-by-default was enabled.
Confirmed ctrl+C keyboard shortcut (as listed on context menu) copied the clean link.
Note - per comment on issue, this should be fully tested on 1.49.x. Can use "Default" (Enabled) case from brave/brave-browser#29177 (comment) as example on what to verify.

Example Example Example
w1 w2 w3

Brave	1.51.64 Chromium: 112.0.5615.39 (Official Build) nightly (64-bit) 
Revision	a0e7b9718a92bcd1cf33b7c95316caff3fc20714-refs/branch-heads/5615@{#753}
OS	Linux
Linux - PASSED

Spot checked that the default for brave://flags/#brave-copy-clean-link-by-default was enabled.
Confirmed ctrl+C copied the clean link (as listed on context menu).Confirmed ctrl+C keyboard shortcut (as listed on context menu) copied the clean link.
Note - per comment on issue, this should be fully tested on 1.49.x. Can use "Default" (Enabled) case from brave/brave-browser#29177 (comment) as an example on what to verify.

Example Example Example
L1 L2 L3

kjozwiak pushed a commit that referenced this pull request Mar 27, 2023
kjozwiak pushed a commit that referenced this pull request Mar 27, 2023
@spylogsster spylogsster deleted the clean-link-osx-off branch May 16, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Copy Clean link hotkeys for OSX
3 participants