1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Simona Badau, Desktop QA]

Attachment: Screenshot1.png

[Affected versions]:
- Nightly 55.0a1

[Affected platforms]:
- All 

[Steps to reproduce]:
1. Launch Firefox 
2. Open Customize (Open Menu -> Customize)
3. Type anything in the URL bar

[Expected result]:
- The text typed in the URL bar should be the same with the text displayed in the One-off-Searched bar

[Actual result]:
- The last typed action is not visible in the One-off-Searched bar

[Regression range]:
Last good revision: 6e191a55c3d23e83e6a2e72e4e80c1dc21516493
First bad revision: 7b74ee1d97dbe7afa04e6b522174be2c529349b9
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6e191a55c3d23e83e6a2e72e4e80c1dc21516493&tochange=7b74ee1d97dbe7afa04e6b522174be2c529349b9

Comment 1

[Simona Badau, Desktop QA]

Attachment: Screenshot2.png

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar.

Review commit: https://reviewboard.mozilla.org/r/131792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131792/

Comment 3

[Drew Willcoxon :adw]

To summarize, what's happening is that the one-offs header is one input event behind the urlbar.  Except for the very first event.  For example (sorry for the ascii art):

urlbar | one-offs header
-------+----------------
f      | f
fo     | f
foo    | fo

After the urlbar is destructed and reconstructed due to entering and exiting customize mode, the input listener added to the urlbar by the one-offs binding remains in place.  Not sure how.  The autocomplete input binding has its own input handler too, and the problem is that after the urlbar is reconstructed, that handler is called after the listener added by the one-offs, presumably because it's added after.  (Or more accurately the input event listener corresponding to the handler is added after.)

I'm not sure why the searchbar doesn't have this problem.

There are a couple of ways to fix this.  (1) In the urlbar's destructor, null out the one-off's textbox property, which removes the input listener.  When the urlbar is reconstructed, it will set the textbox property again, which will cause the one-offs to add the input listener back, behind autocomplete's input handler.

(2) Right now the one-off's textbox setter returns early if val == _textbox, which prevents the input listener from being removed and added.  So just get rid of that early return.

This patch does both.  Option 1 seems like the "right" way, but I don't think there's any need for the early return in the textbox setter, especially since it has caused a weird problem like this.  No need to risk further weird problems.

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8859807 [details]
Bug 1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar.

https://reviewboard.mozilla.org/r/131792/#review135046

Thanks for the explanation in comment 3! I wonder if we should add a short summary of this above the 2 lines you are adding in the urlbar destructor, to avoid someone removing this later without understanding which edge case these lines were added for.

::: browser/base/content/urlbarBindings.xml:127
(Diff revision 1)
>          this.inputField.removeEventListener("mousedown", this);
>          this.inputField.removeEventListener("mousemove", this);
>          this.inputField.removeEventListener("mouseout", this);
>          this.inputField.removeEventListener("overflow", this);
>          this.inputField.removeEventListener("underflow", this);
> +        this.popup.oneOffSearchButtons.popup = null;

Your comment in the bug explains why you are nulling out the textbox property, but is there a specific reason for doing it to the popup property too?

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8859807 [details]
Bug 1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131792/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8859807 [details]
Bug 1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar.

Hmm, your comment got me thinking that it's probably also a good idea not to return early in the popup setter, too.  It doesn't look like there's a problem with popup the way there was with textbox, but potentially there could be since the popup setter also adds event listeners.  Unlike the textbox setter though, the popup setter has another side effect: rebuilding the popup.  But that only happens if the popup is open, and that's unlikely to be the case in practice when the same popup is being set.

So the latest patch removes that, and I'm re-requesting review for that.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8859807 [details]
Bug 1357458 - After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar.

https://reviewboard.mozilla.org/r/131792/#review135256

Comment 8

[Drew Willcoxon :adw]

Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c993698c0d3
After Customization - typed text in the Awesome bar doesn't correspond with the text from One-Off-Searches bar. r=florian

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c993698c0d3

Comment 11

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 55.0a1 (20170424030211) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this was verified in 55 and still affects 54, want to request uplift to beta 54?

Comment 13

[Drew Willcoxon :adw]

No thanks, one-off searches will be enabled on non-Nightly starting in 55, so there's no reason to uplift.