Improve location bar delayed Enter behavior

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Address Bar
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified, firefox51 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Due to the async behavior it's pretty clear we may have some race conditions we are unaware of, indeed we saw bugs reported about the locationbar handling Enter at the wrong time, or not handling it.

I'm going to introduce some stopgaps to the current delayed Enter behavior to improve the situation.
Unfortunately in many cases the reported behaviors are intermittent, so writing automated tests is impossible (we should clearly write them when we actually find plausible causes for these problems).
Whiteboard: [fxsearch]
Blocks: 1240217
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Afaict, these, along with bug 1292310, should fix all the mi-entered cases reported in bug 1240217 and bug 1279864. Though, they don't help with the "perf" problem reported in bug 1279864.

For that I had a third part that was delaying Enter only when we could have autofilled (single word, cursor at the end...) BUT then i remembered about keywords, we don't know if the first typed word is a keyword, so we don't know if we should handle it as such.
Actually, the hack would be working, but only because for keywords we do the work twice, first unified returns the fact something is a keyword, and then the browser again checks if the search text may be a keyword. There's a bug somewhere filed by Blair to remove this double handling.
Due to that, I didn't want to introduce a perf improvement that in future we will have to remove to achieve better code, and I'm throwing that idea away for now.

I'll keep looking into the perf problem apart for bug 1279864 cause I want to check if there's any low hanging fruits that would help us returning faster and let this bug only take care of Enter misbehaviors.

Comment 4

a year ago
mozreview-review
Comment on attachment 8789479 [details]
Bug 1301093 - Part 1: don't wait for a result when we won't get one.

https://reviewboard.mozilla.org/r/77672/#review76394

::: toolkit/components/autocomplete/nsIAutoCompleteController.idl:59
(Diff revision 1)
>     *       Then, implementation of handleText() can access the editor when
>     *       it's not in composing mode. DOM compositionend event is not good
>     *       timing for calling handleText(). DOM input event immediately after
>     *       DOM compositionend event is the best timing to call this.
> +   *
> +   * @return whether this handler started a new search.

Do you mind adding a @return to the handleDelete comment while you're here?
Attachment #8789479 - Flags: review?(adw) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8789480 [details]
Bug 1301093 - Part 2: sanity check that delayed Enter applies to the expected search string.

https://reviewboard.mozilla.org/r/77674/#review76392

::: browser/base/content/urlbarBindings.xml:1046
(Diff revision 1)
>              let rv = this.mController.handleEnter(false, event);
>              delete this._searchStringOnHandleEnter;
>              return rv;
>            }
>  
> -          this.handleEnterWhenGotResult = true;
> +          this.delayedHandleEnterSearchString = this.mController.searchString;

This seems very similar to the this._searchStringOnHandleEnter that I added recently, just slightly above.  Do you think you can use it instead or in other words combine the two into one?
Attachment #8789480 - Flags: review?(adw) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I found another problem, this code:

if (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery) {

was right when we were setting selectedIndex = 0 by default, but now we set it to -1 by default and set to 0 once we get a first result and we know it contains an heuristic result.

That means we are not delaying enter when it's set to -1, but it may be set to -1 only because we are still waiting for a result. So looks like we are not delaying enter when expected.
This may explain why rarely I end up on a visit page instead of the host I was expecting...

We should change the condition to selectedIndex > 0, but then we may not handle cases where we won't get any result (for example autocomplete is disabled), cause selectedIndex would be -1 and gotResultForCurrentQuery would stay false.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
fixing properly delayed enter opened a small can of worms. Since it never worked, some tests were unhappy with that, but that allowed to fix some minor incorrectness here and there.
- First, the delayed handlerEnter needs an event, cause otherwise things like canonize url don't work.
- Note the changes to canonizeurl method are not strictly needed, but it didn't really make sense as it was, the callback was pointless.
- this._ignoreNextSelect was a leftover from previous changes, no more used nor needed.
- The _onSearchBeginChange is explained in the code comment and in comment 8, it ended up being the only fix I could think of, cause we must be able to handle cases where autocomplete doesn't return anything and we don't have an API telling that us
- the browser_canonizeurl change is needed cause now we properly wait to handle enter, so we could autofill, the test didn't expect that and it's not up to it to do (rather we should add another test if we decide canonize should have priority over autofill)
- the completeDefaultIndex change in the controller is because popup invalidation may want to know if we defaultcompleted or not (and it does want to know)
- mDefaultIndexCompleted change is needed cause otherwise we mark as defaultCompleted things that are not 

Most of these changes are covered by existing tests, and indeed they have been uncovered by those.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=809499a50b9f3f914fc8bb3a56796efc6adb7a73
Blocks: 1283329

Comment 14

a year ago
mozreview-review
Comment on attachment 8789480 [details]
Bug 1301093 - Part 2: sanity check that delayed Enter applies to the expected search string.

https://reviewboard.mozilla.org/r/77674/#review77698

Tricky, looks good to me.
Argh, I re-reviewed part 2 when I should have reviewed part 3.  Thanks mozreview.

Comment 16

a year ago
mozreview-review
Comment on attachment 8791338 [details]
Bug 1301093 - Part 3: fix delayed handleEnter regression.

https://reviewboard.mozilla.org/r/78770/#review77700

OK, same comment: tricky, but looks good to me. :-)  Nice tests too.
Attachment #8791338 - Flags: review?(adw) → review+

Comment 17

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/1d99d855d1a9
Part 1: don't wait for a result when we won't get one. r=adw
https://hg.mozilla.org/integration/autoland/rev/eed372e27a7a
Part 2: sanity check that delayed Enter applies to the expected search string. r=adw
https://hg.mozilla.org/integration/autoland/rev/85ec67dd11e5
Part 3: fix delayed handleEnter regression. r=adw
Depends on: 1303346

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1d99d855d1a9
https://hg.mozilla.org/mozilla-central/rev/eed372e27a7a
https://hg.mozilla.org/mozilla-central/rev/85ec67dd11e5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1304027
status-firefox50: --- → fixed
I reproduced this issue  using Fx 48.0a1, build ID: 20160404030231, on Windows 10 x64.
I can confirm the issue is fixed, I used Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04.
I verified on:
- Fx 50.0b6 (20161010144024)
- Fx 51.0a2 (20161011004015

Cheers!
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.