Closed Bug 1301093 Opened 8 years ago Closed 8 years ago

Improve location bar delayed Enter behavior

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox50 --- verified
firefox51 --- verified

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

Bug 1301093 - Part 1: don't wait for a result when we won't get one.
58 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1301093 - Part 2: sanity check that delayed Enter applies to the expected search string.
58 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1301093 - Part 3: fix delayed handleEnter regression.
58 bytes, text/x-review-board-request
adw
: review+
Details
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
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 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 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+
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.
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.
Blocks: 1283329
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 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+
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: