Closed Bug 1105967 Opened 9 years ago Closed 9 years ago

Typing fast in address bar and pressing enter leads to missing end characters

Categories

(Firefox :: Address Bar, defect)

All
Windows 8.1
defect
Not set
normal
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
38.3 - 23 Feb
Tracking Status
firefox39 --- verified

People

(Reporter: Optimizer, Assigned: Unfocused)

References

Details

Attachments

(1 file)

Prerequisites:

Have latest nightly, set browser.urlbar.unifiedcomplete to true

Bug:

Ever since the first entry started getting auto selected in address bar suggestions in latest nightly, if you type , say a search term "jumanji" very fast, most of the time, location bar misses out end few characters and so the search is most likely to be of "juma" or something similar.

This is because the value in the first entry gets updated with a lag and by the time you have pressed Enter, the value is still trying to keep up to the typed value in addressbar.

So in these cases, when you press Enter, the search happens for the value in the first suggestion instead of the typed value in the urlbar.

Ideally, even if the first suggestion is not updated instantaneously, the search should still happen on the typed value in the urlbar


Happens at least on Windows.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Am treating this as separate from bug 1104985.
Assignee: nobody → bmcbride
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → ASSIGNED
Points: --- → 8
Flags: firefox-backlog+
Iteration: --- → 37.1
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
*sigh* This is progressing frustratingly slow. And I don't even know how well it will work in the end.

Strategy is:
* Keep track of whether we have the first result for the current input string.
* If Enter is hit while anything other than the default (ie, first) item is selected, do everything normally.
* If Enter is hit while the default item is selected, and that item is the result of the current input string, do everything normally.
* Otherwise, Enter was hit while the default item is selected but that item corresponds to an earlier search; or there are no items yet. In which case, we wait for the search to yield a result to use.
Iteration: 37.1 → 37.2
Iteration: 37.2 → 37.3
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Attached patch Patch v1Splinter Review
Attachment #8555058 - Flags: review?(paolo.mozmail)
The approach in comment 3 looks good, but I might not be able to do a detailed review of this very soon. On the other hand I know Marco has lots of review requests already, so I might get to this first if no one else is available earlier.
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(In reply to :Paolo Amadini from comment #5)
> The approach in comment 3 looks good, but I might not be able to do a
> detailed review of this very soon. On the other hand I know Marco has lots
> of review requests already, so I might get to this first if no one else is
> available earlier.

No problem - this isn't blocking me or anything else at the moment (I'm primarily concentrating on ReadingList).
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Comment on attachment 8555058 [details] [diff] [review]
Patch v1

Review of attachment 8555058 [details] [diff] [review]:
-----------------------------------------------------------------

There we go!

::: browser/base/content/test/general/browser_autocomplete_enter_race.js
@@ +13,5 @@
> +                                         PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                         "test");
> +  PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword");
> +
> +  yield new Promise(resolve => { waitForFocus(resolve, window) });

nit: unnecessary braces (or add semicolon after statement)

::: browser/base/content/urlbarBindings.xml
@@ +123,5 @@
>          this.inputField.removeEventListener("underflow", this, false);
>        ]]></destructor>
>  
>        <field name="_value">""</field>
> +      <field name="gotFirstResult">false</field>

Maybe "gotResultForCurrentQuery"? This would maybe make it clearer why we're resetting that in onInput.

@@ +124,5 @@
>        ]]></destructor>
>  
>        <field name="_value">""</field>
> +      <field name="gotFirstResult">false</field>
> +      <field name="waitingForFirstResult">false</field>

"handleEnterWhenGotResult"?

@@ +812,5 @@
> +
> +      <method name="handleEnter">
> +        <body><![CDATA[
> +          if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete")) {
> +            return this.mController.handleEnter(false);

Maybe add a comment to say why we don't need to wait in this case.

@@ +815,5 @@
> +          if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete")) {
> +            return this.mController.handleEnter(false);
> +          }
> +
> +          if (this.popup.selectedIndex != 0 || this.gotFirstResult) {

Also a comment here would be nice.
Attachment #8555058 - Flags: review?(paolo.mozmail) → review+
Added a bunch of comments to help explain what's going on and why.

https://hg.mozilla.org/integration/fx-team/rev/6c9f72338190
https://hg.mozilla.org/mozilla-central/rev/6c9f72338190
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
So, how does this differ from bug 1104985?
Flags: needinfo?(bmcbride)
QA Contact: andrei.vaida
(In reply to Marco Bonardo [::mak] -- currently sick :( (delayed answers) from comment #10)
> So, how does this differ from bug 1104985?

Bug 1104985 has hanged a bit, I've updated the summary there.

This bug fixes the async issue globally, and fixes the original symptoms of bug 1104985.

In bug 1104985, I'd like to remove the DB query (keywordQuery) in bug 1104985, since we should be able to just rely on the cache. IIRC, originally that would have make it entirely synchronous, but with your changes to the cache that's no longer the case (?)... but it's still an optimization win to avoid that DB query.
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #11)
> In bug 1104985, I'd like to remove the DB query (keywordQuery) in bug
> 1104985, since we should be able to just rely on the cache. IIRC, originally
> that would have make it entirely synchronous, but with your changes to the
> cache that's no longer the case (?)... but it's still an optimization win to
> avoid that DB query.

Ah sure, that will wait for the new keywords API then (that I hope to complete in this iteration).
(In reply to Girish Sharma [:Optimizer] from comment #0)
> if you type , say a search term "jumanji"
> very fast, most of the time, location bar misses out end few characters and
> so the search is most likely to be of "juma" or something similar.

Define "very fast". I tried to reproduce the bug on affected builds from 2014-11-27, 2014-11-28, 2014-12-02, with no success. I didn't notice any end characters getting lost after typing (reasonably) fast and pressing [enter] on Windows 8.1 (x64) and Windows 7 (x64), with that pref enabled. 

Am I missing something here?
Flags: needinfo?(bmcbride)
unified complete is disabled in nightly, you must enable it by setting browser.urlbar.unifiedcomplete to true (and likely restarting Firefox)
Flags: needinfo?(bmcbride)
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #14)
> unified complete is disabled in nightly, you must enable it by setting
> browser.urlbar.unifiedcomplete to true (and likely restarting Firefox)

The testing mentioned in Comment 13 was made with 'browser.urlbar.unifiedcomplete' set to true, followed by a browser restart.
ok, then I'm setting back the needinfo
Though this bug was very simple to reproduce afair, we disabled the feature cause nightly was unusable.
Flags: needinfo?(bmcbride)
Yea, I could never reliably reproduce this (I couldn't even reliably reproduce it in tests). And I did notice it became harder to reproduce - but I have no idea why. I wouldn't worry too much about reproducing it in older builds - just that this patch didn't inadvertently regress anything else.
Flags: needinfo?(bmcbride)
 (In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #17)
> Yea, I could never reliably reproduce this (I couldn't even reliably
> reproduce it in tests). And I did notice it became harder to reproduce - but
> I have no idea why. I wouldn't worry too much about reproducing it in older
> builds - just that this patch didn't inadvertently regress anything else.

Thank you for clarifying this.

This is now verified fixed on Nightly 39.0a1 (2015-03-11), using Windows 7 (x64), Mac OS X 10.9.5 and Ubuntu 14.04 (x64).
Status: RESOLVED → VERIFIED
Depends on: 1142781
Depends on: 1240217
You need to log in before you can comment on or make changes to this bug.