Closed
Bug 1105967
Opened 10 years ago
Closed 10 years ago
Typing fast in address bar and pressing enter leads to missing end characters
Categories
(Firefox :: Address Bar, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox39 | --- | verified |
People
(Reporter: Optimizer, Assigned: Unfocused)
References
Details
Attachments
(1 file)
(deleted),
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 2•10 years ago
|
||
Am treating this as separate from bug 1104985.
Assignee: nobody → bmcbride
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Points: --- → 8
Flags: firefox-backlog+
Updated•10 years ago
|
Iteration: --- → 37.1
Flags: qe-verify?
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
Assignee | ||
Comment 3•10 years ago
|
||
*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.
Updated•10 years ago
|
Iteration: 37.1 → 37.2
Updated•10 years ago
|
Iteration: 37.2 → 37.3
Updated•10 years ago
|
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8555058 -
Flags: review?(paolo.mozmail)
Comment 5•10 years ago
|
||
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.
Updated•10 years ago
|
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
Assignee | ||
Comment 6•10 years ago
|
||
(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).
Updated•10 years ago
|
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Added a bunch of comments to help explain what's going on and why.
https://hg.mozilla.org/integration/fx-team/rev/6c9f72338190
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Updated•10 years ago
|
Blocks: UnifiedComplete
Updated•10 years ago
|
QA Contact: andrei.vaida
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
(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).
Comment 13•10 years ago
|
||
(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)
Comment 14•10 years ago
|
||
unified complete is disabled in nightly, you must enable it by setting browser.urlbar.unifiedcomplete to true (and likely restarting Firefox)
Flags: needinfo?(bmcbride)
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•