Work around bug 924069 in the Firefox front-end code

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 30
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Follow-up to bug 924069.

Gavin, can you please find an owner for this?
Flags: needinfo?(gavin.sharp)
OS: Mac OS X → Linux
Gavin: ping on the needinfo here!
Ehsan: sounds like you know what needs doing and care about it being done. You sound like a perfect owner!
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Whiteboard: [triage] [defect] p=0
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.
(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?
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
also that bug 924069 doesn't regress :)
Blocks: fxdesktoptriage
No longer blocks: fxdesktopbacklog
Blocks: 981652
Assignee: nobody → ehsan
Attachment #8388554 - Flags: review?(gavin.sharp)
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?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
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? :)
I'm assuming the "select" event is fired synchronously from under setSelectionRange - is that not the case?
The select event is fired asynchronously.
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.
Attachment #8388554 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.)
Flags: needinfo?(gavin.sharp)
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 :(
Flags: needinfo?(gavin.sharp)
Attachment #8388554 - Attachment is obsolete: true
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! :-)
Attachment #8389589 - Flags: review?(gavin.sharp)
Attachment #8389589 - Flags: review?(gavin.sharp) → review+
No longer blocks: fxdesktoptriage
Whiteboard: [triage] [defect] p=0
https://hg.mozilla.org/mozilla-central/rev/86fad085d4b1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
Depends on: 1180080
You need to log in before you can comment on or make changes to this bug.