931904 - Work around bug 924069 in the Firefox front-end code

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[(no longer active)]

Follow-up to bug 924069.

Gavin, can you please find an owner for this?

Comment 1

[(no longer active)]

Gavin: ping on the needinfo here!

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Ehsan: sounds like you know what needs doing and care about it being done. You sound like a perfect owner!

Comment 3

[(no longer active)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> Ehsan: sounds like you know what needs doing and care about it being done.
> You sound like a perfect owner!

That's not really helpful.  You know that I did try to do that (see bug 924069 comment 29), which is why I'm asking for help here from somebody who knows this code.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I'm happy to explain how urlbarBindings.xml/autocomplete.xml interact (urlbarBindings.xml's "urlbar" binding extends autocomplete.xml's "autocomplete" binding). So in autocomplete.xml's selectTextRange, you'd set a private flag on "this", and then in urlbarBinding.xml's "select" handler you could check that flag and ignore the event if it's set.

Comment 5

[(no longer active)]

(In reply to comment #4)
> I'm happy to explain how urlbarBindings.xml/autocomplete.xml interact
> (urlbarBindings.xml's "urlbar" binding extends autocomplete.xml's
> "autocomplete" binding). So in autocomplete.xml's selectTextRange, you'd set a
> private flag on "this", and then in urlbarBinding.xml's "select" handler you
> could check that flag and ignore the event if it's set.

I see, thanks.  Can you please also let me know how I should test for the desired behavior here?  IOW, how can I make sure that my patch doesn't break the existing behavior?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

You want to test that for the selection clipboard on Linux, the behaviors from _getSelectedValueForClipboard (http://hg.mozilla.org/mozilla-central/annotate/53376ef850fc/browser/base/content/urlbarBindings.xml#l471) apply, namely:
- clipboard doesn't include "moz-action:" URLs when copying the URL while a "switch to tab" item is selected
- the clipboard contains the right thing when a partial URL is selected, but includes the scheme (not shown) when the entire URL is selected
- copied value is escaped properly
- parentheses are escaped properly

Comment 7

[:Gavin Sharp [email: [e-mail]]]

also that bug 924069 doesn't regress :)

Comment 8

[(no longer active)]

Attachment: Ignore selections coming from the autocomplete code in urlbarBindings; r=gavin

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8388554 [details] [diff] [review]
Ignore selections coming from the autocomplete code in urlbarBindings; r=gavin

Is it guaranteed that setSelectionRange will fire a select event? I'd feel a little more comfortable if there was a this._ignoreNextSelectEvent = false; after that call in case it somehow doesn't?

Comment 10

[(no longer active)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> Comment on attachment 8388554 [details] [diff] [review]
> Ignore selections coming from the autocomplete code in urlbarBindings;
> r=gavin
> 
> Is it guaranteed that setSelectionRange will fire a select event? I'd feel a
> little more comfortable if there was a this._ignoreNextSelectEvent = false;
> after that call in case it somehow doesn't?

Bug 981652 should make it do so for elements with a frame, which should be fine here.  I'm planning to land these fixes together.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Is the textbox guaranteed to always have a frame when this XBL method is called? What if another "select" listener calls stopPropagation? Can you just add the extra line? :)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I'm assuming the "select" event is fired synchronously from under setSelectionRange - is that not the case?

Comment 13

[(no longer active)]

The select event is fired asynchronously.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8388554 [details] [diff] [review]
Ignore selections coming from the autocomplete code in urlbarBindings; r=gavin

Neil points out that this is going to break autocomplete.xml consumers who aren't using the urlbarBindings.xml version.

To fix that, you'll need to have the urlbarBindings.xml binding override the base autocomplete binding's selectTextRange, and put the new code there, rather than changing autocomplete.xml.

Comment 15

[(no longer active)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #14)
> Comment on attachment 8388554 [details] [diff] [review]
> Ignore selections coming from the autocomplete code in urlbarBindings;
> r=gavin
> 
> Neil points out that this is going to break autocomplete.xml consumers who
> aren't using the urlbarBindings.xml version.

I don't see how it's going to break those, given that they will just have one extra variable stored in the binding's JS object, but ok.

> To fix that, you'll need to have the urlbarBindings.xml binding override the
> base autocomplete binding's selectTextRange, and put the new code there,
> rather than changing autocomplete.xml.

How do I override XBL methods?  Also, what guarantees that the override won't break in the future?  (I assume the answer is nothing.)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Yes you're right, not "break". But it's messy to have a _ignoreNextSelect that does nothing in autocomplete.xml.

You "override" methods in XBL by just copying their implementation to the derived binding, and there's no guarantee that they stay up to date to changes in the base binding :(

Comment 17

[(no longer active)]

Attachment: Ignore selections coming from the autocomplete code in urlbarBindings; r=gavin

Comment 18

[(no longer active)]

Comment on attachment 8389589 [details] [diff] [review]
Ignore selections coming from the autocomplete code in urlbarBindings; r=gavin

Thanks for the review.  BTW what you said reminded me that ages ago I wrote this library to implement XBL overlays in JS, and I did learn about all of this stuff back then!  Lo and behold, it's still out there: <http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/extensions/file/6d210b7ca395/libs/xbl-overlay/xbl-overlay.js>.  Thanks for reminding me! :-)

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86fad085d4b1

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86fad085d4b1