Closed Bug 1563812 Opened 5 months ago Closed 5 months ago

Follow-up: Keyboard navigation through quantumbar suggestions jumps to search providers

Categories

(Firefox :: Address Bar, defect, P1)

69 Branch
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 70
Iteration:
69.4 - Jun 24 - Jul 7
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 69+ verified
firefox67 --- unaffected
firefox68 - wontfix
firefox69 + verified
firefox70 + verified

People

(Reporter: adw, Assigned: adw)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1559179 +++

Bug 1559179 fixed one problem that caused the selection to jump to the one-off search buttons, but there's another problem that also causes it. We end up hitting this case here even though the selected result is not the last result: https://searchfox.org/mozilla-central/rev/9e980da78406a808663ba640773d386a2a02965a/browser/components/search/content/search-one-offs.js#977

The problem bug 1559179 fixed was that numListItems was smaller than it should have been. The problem now is that this.selectedAutocompleteIndex is larger than it should be. this.selectedAutocompleteIndex is just a getter that returns view.selectedIndex, and view.selectedIndex is the selected result's uiIndex -- so uiIndex is not being properly updated to account for stale/fresh results.

I can't reproduce this normally, but I noticed that in the gif that Dustin attached to bug 1559179, there seems to be much more latency fetching history results than I'm seeing. So I added an await new Promise(r => setTimeout(r, 500)) to UnifiedComplete here: https://searchfox.org/mozilla-central/rev/9e980da78406a808663ba640773d386a2a02965a/toolkit/components/places/UnifiedComplete.jsm#1032

And now I can reproduce it often, but still not always.

I think the problem is that _removeStaleRows doesn't call _updateIndices.

We need to update indices after removing stale rows.

:adw, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(adw)
Duplicate of this bug: 1564144
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e20833cc231d
Quantumbar: Fix keyboard navigation after stale results have been removed. r=dao
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Flags: in-testsuite+

Should this be uplifted? The original bug is in 69

Yes, I'll request

Flags: needinfo?(adw)
Regressed by: 1554038

STR

As with bug 1559179, this bug may be hard to manually reproduce. The automated test works similarly to these STR fwiw.

  1. Add some network throttling to make this easier to reproduce:
    1a. Open the Browser Toolbox (Tools > Web Developer > Browser Toolbox).
    1b. Click the Network tab.
    1c. In the second toolbar, click the "No throttling" drop-down menu and select GPRS.
  2. Type "zill" in the urlbar.
  3. Two things should now happen:
    3a. At first, the popup should contain only Mozilla-related sites in your history/bookmarks -- no search suggestions.
    3b. But that only lasts a split second. After that, the popup should update itself so that it contains search suggestions at the top followed by Mozilla-related history/bookmarks. Wait for this second sub-step to happen.
  4. Type "a" (so that "zilla" is in the urlbar).
  5. You should see the same two sub-steps as in step 3 (except of course the results will reflect "zilla", not "zill"). However, in this case, press the down arrow key between those two sub-steps, when the popup contains only Mozilla-related history/bookmarks. You have to be quick, but not too quick.
  6. Keep pressing the down arrow key to select each result, until you get to the search buttons.

Expected: You should be able to select all results in the popup. No results should be skipped.

Flags: qe-verify+

Comment on attachment 9076327 [details]
Bug 1563812 - Quantumbar: Fix keyboard navigation after stale results have been removed.

Beta/Release Uplift Approval Request

  • User impact if declined: Users will continue to experience this bug on 69, i.e., selecting results in the urlbar popup using the keyboard will behave incorrectly sporadically.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see comment 11
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a one-line change (not counting tests), it has an automated test, and I've manually verified it.
  • String changes made/needed: None
Attachment #9076327 - Flags: approval-mozilla-beta?

Dustin, do you think you could try a recent Nightly to verify that this is finally fixed for you? That would be good to confirm.

Flags: needinfo?(dustin)

Drew, I can no longer reproduce this issue. Thanks!

Flags: needinfo?(dustin)
QA Whiteboard: [qa-triaged]

This bug is present on 68, setting flags accordingly

Flags: needinfo?

This also incorporates the small patch to bug 1563073 (landed in 69) that fixes a harmless JS warning.

Comment on attachment 9078474 [details] [diff] [review]
mozilla-release & mozilla-esr68 patch

Beta/Release Uplift Approval Request

  • User impact if declined: Users will continue to experience this bug on 68, i.e., selecting results in the urlbar popup using the keyboard will behave incorrectly sporadically. It would be nice if this could ride along in a dot release, but it's probably not critical.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see comment 11
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a small change, it has an automated test, and I've manually verified it. Note that this patch also incorporates the small patch to bug 1563073 (landed in 69) that fixes a harmless JS warning.
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Even if this isn't approved for release, we should uplift it to ESR because this can be a very visible bug.
  • User impact if declined: Users will continue to experience this bug on ESR 68, i.e., selecting results in the urlbar popup using the keyboard will behave incorrectly sporadically.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a small change, it has an automated test, and I've manually verified it. Note that this patch also incorporates the small patch to bug 1563073 (landed in 69) that fixes a harmless JS warning.
  • String or UUID changes made by this patch: None
Attachment #9078474 - Flags: approval-mozilla-release?
Attachment #9078474 - Flags: approval-mozilla-esr68?

Comment on attachment 9076327 [details]
Bug 1563812 - Quantumbar: Fix keyboard navigation after stale results have been removed.

Approved for 69.0b6.

Attachment #9076327 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Reproduced the issue on affected: 69.0a1 2019-07-07 and verified it as fixed on 70.0a1 2019-07-17 using the STR. from comment 11 on Windows10/Osx 10.14/Ubuntu 16.04. Additionally, I think this fix is also verifiable by using https://bugzilla.mozilla.org/show_bug.cgi?id=1559179#c22.

Verified (using https://bugzilla.mozilla.org/show_bug.cgi?id=1559179#c22) on:

     69.0b6 2019-07-18

on:

     Windows 10
     Ubuntu 16.04
     Mac 10.14.5
Comment on attachment 9078474 [details] [diff] [review]
mozilla-release & mozilla-esr68 patch

Not planning another 68 dot release at this time and this is minor enough that I think we can just let the fix ride with 69 anyway. Approved for 68.1esr, though.
Attachment #9078474 - Flags: approval-mozilla-release?
Attachment #9078474 - Flags: approval-mozilla-release-
Attachment #9078474 - Flags: approval-mozilla-esr68?
Attachment #9078474 - Flags: approval-mozilla-esr68+

Verified the fix with:

      68.1.0esr 2019-07-25 ( https://tools.taskcluster.net/index/gecko.v2.mozilla-esr68.latest.firefox)

on:

      Ubuntu 16.04
      Windows 10 x64
      Mac 10.14.6
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.