Closed Bug 924069 Opened 6 years ago Closed 6 years ago

completion in urlbar is copied to X primary selection

Categories

(Firefox :: Address Bar, defect, P1, major)

27 Branch
x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox27 + verified
firefox28 --- verified

People

(Reporter: devel, Assigned: ehsan)

References

Details

(4 keywords)

Attachments

(3 files, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131006030201

Steps to reproduce:

Type the first bit of an URL in the urlbar for which a completion is available.  Press RET.

E.g. say http://www.google.com is completeable, type http://www.goo RET


Actual results:

The completed bit of the URL ("gle.com") is now in the X clipboard.


Expected results:

The X clipboard's contents should not have been tampered with.
WFM 2013-09-18-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
bug 2013-09-19-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
bug 2013-10-06-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-09-18&enddate=2013-09-19+03%3A03
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Location Bar
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/e8bb95435a54
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918105418
Bad:
http://hg.mozilla.org/mozilla-central/rev/dc09d922d41f
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918164934
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8bb95435a54&tochange=dc09d922d41f


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9892615a9776
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130917221353
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce65d48e182e
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130917223022
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9892615a9776&tochange=ce65d48e182e
Regressed by: 
ce65d48e182e	Andrew Quartey — Bug 850364 - Implement setRangeText for HTMLInputElement. r=ehsan
Blocks: 850364
The location bar is a XUL textbox which is an HTML input element underneath. The cause of this is: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#4527. When the user presses RET to complete the suggested url, the remainder of the appended url is interpreted as a selection. This is in line with what the spec says about HTMLInputElement.setSelectionRange()...see: http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#dom-textarea/input-setselectionrange. 

Although for every "successful" range selection made, a select event is to be fired, are there other cases besides this one where we don't want the select event? In such cases, then a solution might be to check |SetSelectionRange| for success at the call sites and fire the select event if necessary but then again this will be going against the spec. Ehsan?
Flags: needinfo?(ehsan)
I don't understand where this bug is coming from quite yet.  Is it because we dispatch the select event?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> I don't understand where this bug is coming from quite yet.  Is it because
> we dispatch the select event?

Yes.
Duplicate of this bug: 925386
Attached patch fix (obsolete) — Splinter Review
This prevents the select event being dispatched if the caller is a chrome element, such as a XUL textbox in this instance. Perhaps, a similar amendment is needed for HTMLTextAreaElement::SetSelectionRange too.

Also, it is interesting to note that Opera and Chrome do not fire select events for |setRangeText| and |setSelectionRange|. A simple adjustment to dump the test results from http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1 to the console will show this.
Assignee: nobody → andrew.quartey
Attachment #815606 - Flags: review?(ehsan)
Comment on attachment 815606 [details] [diff] [review]
fix

I don't think you want IsCallerChrome here - you probably rather want to check nsContentUtils::IsChromeDoc on the element's ownerDocument, or something like that?
Duplicate of this bug: 925559
Component: Location Bar → Editor
Product: Firefox → Core
Component: Editor → DOM
clipboard is unaffected (unless there is a utility running to set the clipboard to the primary selection).
Summary: completion in urlbar is copied to X clipboard → completion in urlbar is copied to X primary selection
Attached patch fix v2 (obsolete) — Splinter Review
switched nsContentUtils::IsCallerChrome to nsContentUtils::IsChromeDoc, on Gavin's suggestion.
Attachment #815606 - Attachment is obsolete: true
Attachment #815606 - Flags: review?(ehsan)
Attachment #815686 - Flags: review?(ehsan)
I'm not clear why Chrome should be special here.  If a web page were to implement something similar, wouldn't it also use similar methods?  And we still don't want the X primary to be changed.

I don't know the purpose of the "select" event from setSelectionRange().
I wonder whether that is conflicting with an existing event, with a listener that sets the primary?
Duplicate of this bug: 926041
Comment on attachment 815686 [details] [diff] [review]
fix v2

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

We should definitely not special case chrome documents like this.

Can you please attach a C++ and Javascript call stack to the SetSelectionRange call which triggers this bug?  I'd like to understand the conditions of this bug a bit better.
Attachment #815686 - Flags: review?(ehsan) → review-
Keywords: dogfood
> Can you please attach a C++ and Javascript call stack to the
> SetSelectionRange call which triggers this bug?  I'd like to understand the
> conditions of this bug a bit better.

I'm currently reconfiguring my system so will take day or two to get to this.
Attached file Call Stack
Can you please provide a js callstack as well?  I basically want to know what our XBL bindings are doing to trigger this...
Attached file JS backtrace
This should be the JS stack that results in the C++ backtrace found in attachment 818850 [details].
Flags: needinfo?(ehsan)
OK, I finally found where this string is copied to the clipboard: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#757>

It is indeed done from a "select" event handler.  Now, my ideal fix would be to get rid of this "select" handler altogether, and let the selection controller code takes care of this.  This code was added back in bug 440075, but the reasons why that was done may not exist any more.

Dao, what do you think?
Component: DOM → Location Bar
Flags: needinfo?(ehsan) → needinfo?(dao)
Product: Core → Firefox
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> This code was added back in bug 440075, but the reasons why that was done may not
> exist any more.

They still exist - the location bar has custom copying behavior. We don't want the selection copy in the common case to use the default behavior of copying the selected text.
(In reply to comment #20)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> > This code was added back in bug 440075, but the reasons why that was done may not
> > exist any more.
> 
> They still exist - the location bar has custom copying behavior. We don't want
> the selection copy in the common case to use the default behavior of copying
> the selected text.

What are the required behaviors when copying?  I tested things such as escape sequences in URLs and internationalized URLs and they all seemed to work fine with the "select" event handler removed.
(In reply to comment #22)
> The set of things described at
> http://hg.mozilla.org/mozilla-central/annotate/c6fc35c53c37/browser/base/content/urlbarBindings.xml#l475

OK, can you suggest an alternative solution please?
Why was this moved from Core to Firefox? I don't see an answer to this question:

(In reply to Karl Tomlinson (:karlt) from comment #12)
> If a web page were to
> implement something similar, wouldn't it also use similar methods?  And we
> still don't want the X primary to be changed.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #24)
> Why was this moved from Core to Firefox?

Because it's a bug in the front-end code in that it incorrectly assumes that the "select" event is only dispatched for user initiated selections.

> I don't see an answer to this
> question:
> 
> (In reply to Karl Tomlinson (:karlt) from comment #12)
> > If a web page were to
> > implement something similar, wouldn't it also use similar methods?  And we
> > still don't want the X primary to be changed.

Define "something similar".  The problem here is that the front-end code calls input.setSelectionRange(), and somewhere else it has a "select" handler which, unaware of that call, tries to copy the selection to the primary selection clipboard.  Now, if a web page relies on the "select" event only coming from the user, then it will break in similar ways.  If and when we find such a web page, we either need to evangelize to the website, or take this discussion back to the spec and try to get everyone to agree to not dispatch the select event for programatic selection changes.  But for now I see no good reason to not dispatch the select event instead of fixing the front-end code.
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> But for now I see no good reason to not dispatch the select event instead of fixing
> the front-end code.

What's the advantage to dispatching the select event? I.e. what motivated the change in behavior introduced by bug 850364? "Adhere to the spec" on its own isn't a great answer in the face of bugs like these.
(In reply to comment #26)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > But for now I see no good reason to not dispatch the select event instead of fixing
> > the front-end code.
> 
> What's the advantage to dispatching the select event? I.e. what motivated the
> change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> isn't a great answer in the face of bugs like these.

Being more consistent with the rest of the HTMLInputEleement APIs for selection.  The old behavior of dispatching the select event only some of the time was indeed very weird, and this is an attempt at fixing that.  Now if we hit a web compat issue with this, then the conversation would be different, but a Firefox front-end compat issue is just not going to be a convincing argument to change the spec here.  ;-)

I agree that this bug is serious, but let's not lose sight of the fact that the front-end code is making incorrect assumptions here.  Can we just fix the front-end code to not do that?
Fair enough. It really boils down to whether the front end code is likely to be the only code making this assumption. It sounds likely, given that other browsers apparently have this same behavior. I don't have any great ideas, short of a flag set in the binding across calls to setSelectionRange, which is a bit gross...
(In reply to comment #28)
> Fair enough. It really boils down to whether the front end code is likely to be
> the only code making this assumption. It sounds likely, given that other
> browsers apparently have this same behavior.

Chrome and Safari don't currently dispatch the "select" event when you call setSelectionRange(), so I would expect us to be the discoverer of the potential web compat issues here (but that's off topic for this bug.)

> I don't have any great ideas,
> short of a flag set in the binding across calls to setSelectionRange, which is
> a bit gross...

Yeah, I was hoping for something better, but maybe there is no better solution.  (Note that I did take a quick stab at writing such a fix but part of the code lives in autocomplete and the other lives in urlbarBindings.xml, and it's not clear to me how they're supposed to interact.)
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #27)
> (In reply to comment #26)
> > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > > But for now I see no good reason to not dispatch the select event instead of fixing
> > > the front-end code.
> > 
> > What's the advantage to dispatching the select event? I.e. what motivated the
> > change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> > isn't a great answer in the face of bugs like these.
> 
> Being more consistent with the rest of the HTMLInputEleement APIs for
> selection.  The old behavior of dispatching the select event only some of
> the time was indeed very weird, and this is an attempt at fixing that.  Now
> if we hit a web compat issue with this, then the conversation would be
> different, but a Firefox front-end compat issue is just not going to be a
> convincing argument to change the spec here.  ;-)

Well, if parts of the Firefox UI can be regarded as role models for desktop-like applications using web technology, and if the goal is to enable all kinds of desktop-like applications on the web, then us running into such issues seems relevant.

> I agree that this bug is serious, but let's not lose sight of the fact that
> the front-end code is making incorrect assumptions here.

Not quite. It makes assumptions that used to be true, until bug 850364 changed behavior.
(In reply to comment #30)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #27)
> > (In reply to comment #26)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > > > But for now I see no good reason to not dispatch the select event instead of fixing
> > > > the front-end code.
> > > 
> > > What's the advantage to dispatching the select event? I.e. what motivated the
> > > change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> > > isn't a great answer in the face of bugs like these.
> > 
> > Being more consistent with the rest of the HTMLInputEleement APIs for
> > selection.  The old behavior of dispatching the select event only some of
> > the time was indeed very weird, and this is an attempt at fixing that.  Now
> > if we hit a web compat issue with this, then the conversation would be
> > different, but a Firefox front-end compat issue is just not going to be a
> > convincing argument to change the spec here.  ;-)
> 
> Well, if parts of the Firefox UI can be regarded as role models for
> desktop-like applications using web technology, and if the goal is to enable
> all kinds of desktop-like applications on the web, then us running into such
> issues seems relevant.

There is nothing here which prevents people from writing desktop-like applications on the web.  Can we please stay on topic here?

> > I agree that this bug is serious, but let's not lose sight of the fact that
> > the front-end code is making incorrect assumptions here.
> 
> Not quite. It makes assumptions that used to be true, until bug 850364 changed
> behavior.

I consider the fact that we were not dispatching "select" before a bug, but sure, I'm more interested in a fix here.  :-)
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #31)
> There is nothing here which prevents people from writing desktop-like
> applications on the web.

I didn't say "prevent". It's an issue we could work around (as discussed in comment 28 and comment 29) and presumably web content could work around it similarly. However this misses the point that it might be better if the issue wouldn't exist in the first place.

> Can we please stay on topic here?

I'm confused. You said a Firefox front-end compat issue wouldn't matter, I argued against that, and now you're telling me that argument is off-topic?
(In reply to comment #32)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #31)
> > There is nothing here which prevents people from writing desktop-like
> > applications on the web.
> 
> I didn't say "prevent". It's an issue we could work around (as discussed in
> comment 28 and comment 29) and presumably web content could work around it
> similarly. However this misses the point that it might be better if the issue
> wouldn't exist in the first place.

Yes, in an ideal world it wouldn't exist in the first place.

> > Can we please stay on topic here?
> 
> I'm confused. You said a Firefox front-end compat issue wouldn't matter, I
> argued against that, and now you're telling me that argument is off-topic?

I did not say that it doesn't matter, I said it's not good enough of a reason to change the HTML spec.  We may reconsider dispatching this event in case we find lots of web compat issue caused by it, but we still have no evidence that such issues exist in practice.

What's the next step here?
This is an extremely annoying bug that disrupts my workflow several times per day.
Can we please backout the offending patch while this bug is being worked on?  Thanks.
Severity: normal → major
Priority: -- → P1
The only reason we're having this lengthy discussion is that |input.setRangeText| had to dispatch the select event and we duly moved it to setSelectionRange. Currently, every |input.selection{Start|End} = someValue;| will also dispatch the event in the present form since they depend on setSelectionRange for implementation. The question here then becomes: is this what the spec intended? I suspect there's a valid reason why other browsers skipped this part of the spec.

Also, others have reservations about removing the location bar selection handler and i agree - we might be playing whack-a-mole with future issues because of that. I'd suggest we instead remove the dispatching of the select event temporarily to satisfy users and in the interim remove the handler and move over its responsibilities to the selection controller, in a way the dispatching future events doesnt interfere with it (easier-said-than-done but doable). Is this agreeable, Ehsan?
Flags: needinfo?(ehsan)
I still haven't seen any evidence that this is a bigger problem than just this one bug, which should be easy to work around, but I was expecting that the front-end folks can provide a quick workaround which hasn't happened yet.  I'll write a patch to change where the select event is being dispatched for now, since I'm going away and want to resolve this before that.
Flags: needinfo?(ehsan)
Assignee: andrew.quartey → ehsan
Attached patch Patch (v1) (obsolete) — Splinter Review
I'd like to land this today, so fast reviews are highly appreciated!
Attachment #815686 - Attachment is obsolete: true
Attachment #823344 - Flags: review?(jst)
Attachment #823344 - Flags: review?(bugs)
Comment on attachment 823344 [details] [diff] [review]
Patch (v1)

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

This will need to be changed too: http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1
(In reply to comment #38)
> Comment on attachment 823344 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=823344
> Patch (v1)
> 
> Review of attachment 823344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will need to be changed too:
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1

Why?
> Why?

Since SetSelectionRange no longer dispatches the select event, it shouldn't be counted in places like this and others:

 //subcase: selection{Start|End} < end
165       elem.value = "0123456789";
166       elem.setSelectionRange(4, 5);
167       elem.setRangeText("QRST", 2, 9, "preserve");
168       is(elem.value, "01QRST9", msg + ".value == \"01QRST9\"");
169       is(elem.selectionStart, 2, msg + ".selectionStart == 2, with \"preserve\"");
170       is(elem.selectionEnd, 6, msg + ".selectionEnd == 6, with \"preserve\"");
171       expectedNumOfSelectCalls += 2;
You're right!
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #823344 - Attachment is obsolete: true
Attachment #823344 - Flags: review?(jst)
Attachment #823344 - Flags: review?(bugs)
Attachment #823366 - Flags: review?(jst)
Attachment #823366 - Flags: review?(bugs)
Comment on attachment 823366 [details] [diff] [review]
Patch (v2)

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

::: content/html/content/test/forms/test_set_range_text.html
@@ +96,5 @@
>        is(elem.selectionStart, 1, msg + ".selectionStart == 1");
>        is(elem.selectionEnd, 4, msg + ".selectionEnd == 4");
>        elem.setRangeText("mnk");
>        is(elem.value, "0mnk6789ABCDEF", msg + ".value == \"0mnk6789ABCDEF\"");
> +      expectedNumOfSelectCalls += 1;

Should be expectedNumOfSelectCalls += 2; here.
Attached patch Patch (v3)Splinter Review
Yeah, the test wasn't resilient to the case where too many "select" events are dispatched.  This patch fixes that too.
Attachment #823366 - Attachment is obsolete: true
Attachment #823366 - Flags: review?(jst)
Attachment #823366 - Flags: review?(bugs)
Attachment #823388 - Flags: review?(jst)
Attachment #823388 - Flags: review?(bugs)
Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

So this is explicitly against what HTML spec says.
I think we need to test this on IE too. I'd prefer keeping the consistency
HTML spec has for select events.
Attachment #823388 - Flags: review?(bugs) → review-
FWIW, IE seems to dispatch select on this test case: <http://people.mozilla.org/~eakhgari/selectevent.html>
Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

Ok, so we added the "select" event during FF27, but don't have
time to fix browser chrome... So this is effectively a backout. Sounds ok.
But please file a bug to re-enable select event and fix browser chrome.
Attachment #823388 - Flags: review?(jst)
Attachment #823388 - Flags: review-
Attachment #823388 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831

Filed bug 931904 as a follow-up to work around this in the front-end code.
https://hg.mozilla.org/mozilla-central/rev/0af6a04d5831
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #48)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831
> 
> Filed bug 931904 as a follow-up to work around this in the front-end code.

Is this ready to be uplifted given its tracking Firefox 27 ?
Duplicate of this bug: 923835
Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 850364
User impact if declined: See comment 0
Testing completed (on m-c, etc.): m-c/locally
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: none
Attachment #823388 - Flags: approval-mozilla-aurora?
(In reply to comment #50)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #48)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831
> > 
> > Filed bug 931904 as a follow-up to work around this in the front-end code.
> 
> Is this ready to be uplifted given its tracking Firefox 27 ?

Yes, sorry this slipped when I was on vacation.
Attachment #823388 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on Firefox 27 beta 4 (build ID: 20140109165205).
Cornel, please also verify this in the latest Aurora build.
Keywords: verifyme
Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0

Confirming this is fixed on latest Audora (build ID: 20140115004003).
Marking as verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 981652
You need to log in before you can comment on or make changes to this bug.