Reassign Ctrl/Cmd + C default hotkey to copy clean urls #16764

spylogsster · 2023-01-20T10:20:06Z

Resolves brave/brave-browser#26761

Copy Clean Link is default for urls:

Copy text:
Win:

Mac:

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:

Select url in the address bar and check context menu for the address bar, it should show default hotkey Ctrl+c for Copy clean link item, try to press Ctrl+c hotkey, it should strip parameters due to copy clean link database. Selecting Copy item from context menu should copy selected url without changes.
Select text (not url) in the address bar and check context menu for the address bar, it should show default hotkey Ctrl+c for Copy item, Copy clean link should not be visible

goodov

lgtm.

browser/ui/views/omnibox/brave_omnibox_view_views.cc

fmarier · 2023-01-20T18:09:01Z

Is there a way to hide "copy clean link" entirely when the selection is just text and not a URL?

fmarier · 2023-01-22T06:10:14Z

browser/ui/views/omnibox/brave_omnibox_view_views_browsertest.cc

+  ])");
+  const std::string test_url(
+      "https://dev-pages.bravesoftware.com/clean-urls/"
+      "exempted/"


It's a little weird to be using exempted/ in these test cases because the JSON config we supply here does not in fact exempt this URL.

I'd suggest removing /exempted/ in these three new test cases to reduce potential confusion.

github-actions · 2023-01-22T16:13:42Z

⚠️ PR head is an unsigned commit
commit: 8e8935b36851c1268adb7d64397649172f2192e9
reason: unsigned
Please follow the handbook to configure commit signing
cc: @invalid-email-address

github-actions · 2023-01-22T16:13:55Z

⚠️ PR head is an unsigned commit
commit: 8e8935b36851c1268adb7d64397649172f2192e9
reason: unsigned
Please follow the handbook to configure commit signing
cc: @invalid-email-address

spylogsster · 2023-01-22T16:14:11Z

Is there a way to hide "copy clean link" entirely when the selection is just text and not a URL?

discussed in Slack, it is hidden when simple text selected, updated screenshots in the description

github-actions · 2023-01-22T16:15:33Z

⚠️ PR head is an unsigned commit
commit: 15c7cc51edeaf811277337c932ed6171e8307445
reason: unsigned
Please follow the handbook to configure commit signing
cc: @spylogsster

simonhong

++

simonhong · 2023-01-25T01:27:30Z

browser/ui/views/omnibox/brave_omnibox_view_views.cc

+  bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();
+  if (is_url) {


Suggested change

bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();

if (is_url) {

if (SelectedTextIsURL()) {

smehrjerdian · 2023-03-18T17:06:16Z

I don't understand why this feature was added to the latest version? Why would we want to copy the clean link when using the basic CMD + C shortcut for copying text off a page?

You can see here the community considers it a bug? Is there a way for us to revert this behavior back to the default behavior we are all used to of CMD+C copying text?

https://community.brave.com/t/keyboard-shortcuts-cmd-c-copy-clean-link-issue/477306

smehrjerdian · 2023-03-18T17:08:34Z

Is there a way to hide "copy clean link" entirely when the selection is just text and not a URL?

I agree with this suggestion? why would we want to copy the clean link when we are highlighting text and clearly trying to copy the text not the URL? I now have to use control + C to copy text and cmd + V to paste it?

fmarier · 2023-03-20T19:02:37Z

@smehrjerdian We will be fixing the bugs that users are running into in a hotfix coming shortly (likely in a day or two).

Additionally, we are working on a feature flag that will let users restore the default keyboard shortcuts: brave/brave-browser#29177 A proper UI setting will come later.

smehrjerdian · 2023-03-20T19:08:06Z

Amazing thanks for the update @fmarier

sbusso · 2023-03-23T01:46:37Z

This is a substantial UX change from the universal behavior of cmd+c, which should copy the selected content. I have been wondering for a few days why I got the correct content copied. I understand some users would like a shortcut to copy the link cleanly, but the default behavior of cmd+c should not be changed.

0xhashmap · 2023-03-30T00:11:00Z

This is still an issue with the new update. The only time cmd+c works properly is when first loading a page or refreshing. If you're navigating around and copy something, the stupid clean link thing happens. This is the most annoying behavior ever. I can't believe someone thought this was a good idea.

fmarier · 2023-03-30T00:47:51Z

@0xhashmap What version of the browser are you using? 1.49.132 is the one that has cmd+C mapped to regular copy.

0xhashmap · 2023-03-30T01:26:39Z

I updated a few days ago, but I guess 1.49.132 was released since. I'm now on the latest version. Looks like it's fixed. Thank you.

spylogsster requested review from fmarier, bsclifton and simonhong January 20, 2023 10:20

spylogsster requested a review from a team as a code owner January 20, 2023 10:20

spylogsster self-assigned this Jan 20, 2023

spylogsster mentioned this pull request Jan 20, 2023

Reassign Copy keyboard shortcut to 'copy clean link' #15968

Closed

25 tasks

goodov approved these changes Jan 20, 2023

View reviewed changes

browser/ui/views/omnibox/brave_omnibox_view_views.cc Show resolved Hide resolved

spylogsster force-pushed the brave-26761-omnibox branch 2 times, most recently from 1fa541c to 1c1cca6 Compare January 20, 2023 14:55

Reassign Ctrl/Cmd + C default hotkey to copy clean urls

8135599

spylogsster force-pushed the brave-26761-omnibox branch from 1c1cca6 to 8135599 Compare January 20, 2023 21:14

fmarier reviewed Jan 22, 2023

View reviewed changes

spylogsster force-pushed the brave-26761-omnibox branch from 8e8935b to 15c7cc5 Compare January 22, 2023 16:15

Corrected test urls

1286696

spylogsster force-pushed the brave-26761-omnibox branch from 15c7cc5 to 1286696 Compare January 22, 2023 16:16

fmarier approved these changes Jan 23, 2023

View reviewed changes

simonhong approved these changes Jan 25, 2023

View reviewed changes

spylogsster merged commit a49f373 into master Jan 25, 2023

spylogsster deleted the brave-26761-omnibox branch January 25, 2023 04:55

github-actions bot added this to the 1.49.x - Nightly milestone Jan 25, 2023

spylogsster mentioned this pull request Jan 27, 2023

Cannot copy/paste autocompleted url brave/brave-browser#28103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reassign Ctrl/Cmd + C default hotkey to copy clean urls #16764

Reassign Ctrl/Cmd + C default hotkey to copy clean urls #16764

spylogsster commented Jan 20, 2023 •

edited

Loading

goodov left a comment

fmarier commented Jan 20, 2023

fmarier Jan 22, 2023

spylogsster Jan 22, 2023

github-actions bot commented Jan 22, 2023

github-actions bot commented Jan 22, 2023

spylogsster commented Jan 22, 2023 •

edited

Loading

github-actions bot commented Jan 22, 2023

simonhong left a comment

simonhong Jan 25, 2023

smehrjerdian commented Mar 18, 2023

smehrjerdian commented Mar 18, 2023

fmarier commented Mar 20, 2023

smehrjerdian commented Mar 20, 2023

sbusso commented Mar 23, 2023

0xhashmap commented Mar 30, 2023 •

edited

Loading

fmarier commented Mar 30, 2023

0xhashmap commented Mar 30, 2023

		bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();
		if (is_url) {

	bool is_url = const_cast<BraveOmniboxViewViews*>(this)->SelectedTextIsURL();
	if (is_url) {
	if (SelectedTextIsURL()) {

Reassign Ctrl/Cmd + C default hotkey to copy clean urls #16764

Reassign Ctrl/Cmd + C default hotkey to copy clean urls #16764

Conversation

spylogsster commented Jan 20, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

goodov left a comment

Choose a reason for hiding this comment

fmarier commented Jan 20, 2023

fmarier Jan 22, 2023

Choose a reason for hiding this comment

spylogsster Jan 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2023

github-actions bot commented Jan 22, 2023

spylogsster commented Jan 22, 2023 • edited Loading

github-actions bot commented Jan 22, 2023

simonhong left a comment

Choose a reason for hiding this comment

simonhong Jan 25, 2023

Choose a reason for hiding this comment

smehrjerdian commented Mar 18, 2023

smehrjerdian commented Mar 18, 2023

fmarier commented Mar 20, 2023

smehrjerdian commented Mar 20, 2023

sbusso commented Mar 23, 2023

0xhashmap commented Mar 30, 2023 • edited Loading

fmarier commented Mar 30, 2023

0xhashmap commented Mar 30, 2023

spylogsster commented Jan 20, 2023 •

edited

Loading

spylogsster commented Jan 22, 2023 •

edited

Loading

0xhashmap commented Mar 30, 2023 •

edited

Loading