Closed Bug 1328025 Opened 3 years ago Closed 3 years ago

Ctrl+C in urlbar only copies the last range of text selection

Categories

(Firefox :: Address Bar, defect)

3.6 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: mats)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open http://example.org/#hello,world! in a new tab
2. Select "org" then "world" in urlbar (suggested way to do so is [3] in the end of this comment)
3. Press Ctrl+C to copy selected text
4. Press Ctrl+A, then Ctrl+V to paste the text copied in Step 3 (or paste it somewhere else)

AR:  "world" appears in urlbar, what means that only second range was copied to clipboard in Step 3
ER:  "orgworld" should be copied to clipboard in Step 3



This was regressed twice:   [1] (2007-07-05 - 2007-07-06), then [2] (2012-06-07 - 2012-06-08)

[1] "Ctrl+C worked OK" -> "Ctrl+C copies only 1st range"
There's no regression range, but it's possible to search bugs on Bugzilla (link below).
Probably it's bug 386971.
> https://bugzilla.mozilla.org/buglist.cgi?list_id=13089055&resolution=---&resolution=FIXED&resolution=INVALID&resolution=WONTFIX&resolution=DUPLICATE&resolution=WORKSFORME&resolution=INCOMPLETE&resolution=SUPPORT&resolution=EXPIRED&resolution=MOVED&chfieldto=2007-07-06&query_format=advanced&chfield=resolution&chfieldfrom=2007-07-05&chfieldvalue=FIXED&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED

[2] "Ctrl+C copies only 1st range" -> "Ctrl+C copies only 2nd range"
Here's regression range, but it's unclear what bug caused this cange:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e4c2abb9fc9&tochange=22bb7d46bb23

[3] Explanation of STR_1 Step 2:
  2.1. Move mouse between "." and "o" in string ".org/", hold left mouse button
  2.2. Move mouse between "g" and "/" in string ".org/", release left mouse button
  2.3. Hold Ctrl key
  2.4. Move mouse between "," and "w" in string ",world!", hold left mouse button
  2.5. Move mouse between "d" and "!" in string ",world!", release left mouse button
  2.6. Release Ctrl key



Note:
 1) Ctrl+X never worked with multiple ranges in urlbar properly, but that's because Ctrl+X was
    broken in ANY <input> before 2007-07-05, and then urlbar case was regressed (see [1]).
 2) Delete key works OK in urlbar with multiple ranges.
    But on 2007-07-05 it was broken, just like Ctrl+X, in ANY <input>.
No longer blocks: 1277113
Component: Untriaged → Editor
Product: Firefox → Core
I think bug 366797 is the culprit.
Blocks: 366797
Component: Editor → Location Bar
Product: Core → Firefox
Attached patch wip (obsolete) — Splinter Review
This appears to fix it for me.

Do we have a test suite for this sort of thing that I can extend?
Flags: needinfo?(dao+bmo)
(In reply to Mats Palmgren (:mats) from comment #2)
> Do we have a test suite for this sort of thing that I can extend?

browser_urlbarCopying.js
Flags: needinfo?(dao+bmo)
Assignee: nobody → mats
Attachment #8823045 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8823125 - Flags: review?(mak77)
Comment on attachment 8823126 [details] [diff] [review]
Add tests for multi-range Selection in URL bar.

Review of attachment 8823126 [details] [diff] [review]:
-----------------------------------------------------------------

feel free to merge the patches, I don't need them separated for the review.

::: browser/base/content/test/urlbar/browser_urlbarCopying.js
@@ +84,5 @@
> +    copyExpected: "em"
> +  },
> +  {
> +    copyVal: "<exam><ple.com>",
> +    copyExpected: "example.com"

This is an interesting edge-case. Since the selection starts from the beginning and it has no interruptions, one may expect it to work exactly like "<example.com>"
It actually doesn't happen, cause we check this.selectionStart > 0 rather than this.editor.selection.getRangeAt(0).startOffset > 0

The problem is that if we'd start also supporting this case, we'd need to add code to check that the selection ranges are adjacent, that will likely cause us to add complex code to support an edge case. It's feasible, but is it worth it? Imo it's not.

Moreover if you select something further in the string AND then select at the beginning, the "this.selectionStart > 0" check won't be true, so the suggested change looks wrong:
<ex>ample.c<om> should not be handled as a possible url only because "ex" has been selected after "om"... this would end up being copied as "http://exom", that is clearly wrong.
Again, it would require to check ranges adjacency.

To sum up, I think the proposed fix is not ok, cause in some cases it will pass the selectionStart check when it should not. And I don't think it's worth adding code complication to support every possible edge-case.

What I suggest instead, is to do a very early bail-out, like:

// For simplicity exclude edge-cases like multiple selection ranges.
if (this.editor.selection.rangeCount > 1) {
  return this.editor.selection.toString();
}

At least it will be clear we are not handling that edge-case by will, and not because we forgot about it.

@@ +224,5 @@
>    waitForClipboard(targetValue, function() {
>      gURLBar.focus();
>      if (copyVal) {
> +      let offsets = [];
> +      while (true) {

I suspect this could be rewritten in a more readable way using a regex to extract the ranges.
For example:

let re = /<([^>]+)>/g;
let match = null;
while((match = re.exec("a<n> e<xamp>l<e>"))) {
  console.log(match[1]);
  console.log(match.index);
  console.log(match[1].length);
}
Attachment #8823126 - Flags: review?(mak77) → review-
Comment on attachment 8823125 [details] [diff] [review]
Unbreak copying of multi-range Selection in URL bar.

the other review comment covers this as well.
Attachment #8823125 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #6)
> let re = /<([^>]+)>/g;
> let match = null;
> while((match = re.exec("a<n> e<xamp>l<e>"))) {
>   console.log(match[1]);
>   console.log(match.index);
>   console.log(match[1].length);
> }

That gives the wrong indices though; we want the resulting indices *after*
the <> are removed so we need to remove those as we go (as the current
patch does).  But that skips the last match for some reason:

var re = /<([^>]+)>/g;
var match = null;
var copyVal = "a<n> e<xamp>l<e>";
while((match = re.exec(copyVal))) {
  console.log(match[1]);
  console.log(match.index);
  console.log(match[1].length);
  copyVal = copyVal.replace("<", "").replace(">", "");
}
(In reply to Marco Bonardo [::mak] from comment #6)
> At least it will be clear we are not handling that edge-case by will, and
> not because we forgot about it.

Fair enough, just returning the string value without any URI fixups
seems fine to me.

The test gives the correct indices and gives a nice error message
if there are unbalanced <>, just like the current code does, so it
doesn't seem worth the time to polish the syntax further.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd649b6e2aad05a1845134752c1b439f13a88ea1
Attachment #8823125 - Attachment is obsolete: true
Attachment #8823126 - Attachment is obsolete: true
Attachment #8824777 - Flags: review?(mak77)
(In reply to Mats Palmgren (:mats) from comment #8)
> That gives the wrong indices though; we want the resulting indices *after*
> the <> are removed so we need to remove those as we go (as the current
> patch does).  But that skips the last match for some reason:

I think it's because you modify the string as you go, and likely the regex keeps buffered indices into it. Btw, I didn't intend to attach perfectly working code, just an idea, with some code it can definitely be done, for example:

var copyVal = "a<n> e<xamp>l<e>";
var re = /<([^>]+)>/g;
var matches = [];
var match = null;
while((match = re.exec(copyVal))) {
  matches.push({ match: match[1],
                 index: match.index - matches.length * 2 });
}
JSON.stringify(matches);

Btw, it's not critical, was just trying to clean it up a little bit.
Comment on attachment 8824777 [details] [diff] [review]
Unbreak copying of multi-range Selection in URL bar.

Review of attachment 8824777 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8824777 - Flags: review?(mak77) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03cfda0cdbbb
Unbreak copying of multi-range Selection in URL bar.  r=mak
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1d6ce205f0
Unbreak copying of multi-range Selection in URL bar.  r=mak
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0180dfa0f7d8
Backed out changeset 03cfda0cdbbb for breaking testAccessibleCarets.js. r=backout
https://hg.mozilla.org/mozilla-central/rev/2a1d6ce205f0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Did this land or not?! the backout is confusing?
Flags: needinfo?(cbook)
It was briefly backed out (on m-i) due to another problem, and then relanded.
Flags: needinfo?(cbook)
Flags: in-testsuite+
Is this worth considering for Aurora uplift?
Flags: needinfo?(mats)
Version: Trunk → 3.6 Branch
Nah, I don't think so.  It's extremely rare that users know multi-range selection
even exist. And the ones who do is unlikely to use it in the URL bar.  And there's
an easy workaround: CTRL+A then edit the text somewhere else. :-)
Flags: needinfo?(mats)
Blocks: 1348644
No longer blocks: 1348644
Depends on: 1402715
You need to log in before you can comment on or make changes to this bug.