1697517 - Tab to search - Search with google shows up in the wrong position

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Pablo]

Attachment: tabtosearch.jpg

description
when using tab to search really fast, the position in the url bar is not correct

Tested on
Windows10 64bit, Ubuntu 20 64bit, MacOS 10.15
Firefox Nightly 88.0a1 = reproducible
Firecox Beta 87.0b8 = Reproducible
Firefox Release 86 = not reproducible.

steps to reproduce

1. launch fx with Clean profile, open 3 tabs, in each enter www.google.com and hit enter.
2. on the 4th tab type "g" and hit tab key (notice the blue icon search with google is on second place)
3. hit backspace key ( to get out of search mode)
4. type again REALLY FAST "g" + tab key

actual results
the search with google is out of order

expected results
search with google should be second position

notes: if you type g+TAB slow it shows correctly, if i type it fast it changes the order.

Comment 1

[Drew Willcoxon :adw]

Pablo, I suspect this is due to bug 1695666. Could you verify please? I'll also try to verify when I have time.

Comment 2

[Pablo]

Hi Marco and Drew

I just finished the mozregression and this is the info:

Build info

build_date: 2021-03-02 00:42:59.270000
build_url: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UrPFK_FKRMC3pe7bFMqenQ/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: bb160ca0dda6d96090bc43a95e0e452077aeb76f
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bb160ca0dda6d96090bc43a95e0e452077aeb76f&tochange=33c7b633ada258d3d12c0ecbe9e5bd2d15671db6
repo_name: autoland
task_id: UrPFK_FKRMC3pe7bFMqenQ

---

Log

Bug 1695666 - To reduce urlbar view flicker, don't reuse rows with different suggested indexes. r=harry

Differential Revision: https://phabricator.services.mozilla.com/D106832

Comment 3

[Julien Cristau [:jcristau]]

Would backing out bug 169566 be an option for 87, since it sounds like this regression affects non-QS?

Comment 4

[Drew Willcoxon :adw]

Yes (bug 1695666), but it will make the QS experience worse.

Comment 5

[Drew Willcoxon :adw]

There's a really quick fix we could do that only applies the fix to bug 1695666 to the last row. That should fix this regression while keeping the QS fix since QS rows are always last.

Comment 6

[Drew Willcoxon :adw]

Attachment: Bug 1697517 - Apply suggestedIndex reuse logic only to the final row in the view to avoid breaking tab to search.

This is a wallpaper patch that keeps the fix to bug 1695666 for quick suggest
results, since they're always last, and also fixes bug 1697517. Ultimately we
should find a better fix.

Comment 7

[Drew Willcoxon :adw]

If we can't take the quick fix I just posted for 87, then I think we should back out bug 1695666 on 87 since it can break tab to search for everyone, vs. the smaller upside that it prevents visual flicker for people who will be in the QS experiment.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68ec2f083bbb
Apply suggestedIndex reuse logic only to the final row in the view to avoid breaking tab to search. r=harry

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/68ec2f083bbb

Comment 10

[Drew Willcoxon :adw]

[Tracking Requested - why for this release]:

This is a regression in tab-to-search on 87. We should either take this patch or back out bug 1695666. I think the patch is safe and we should take it, and I'll request uplift next.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 9208504 [details]
Bug 1697517 - Apply suggestedIndex reuse logic only to the final row in the view to avoid breaking tab to search.

Beta/Release Uplift Approval Request

• User impact if declined: If we don't take this patch or back out bug 1695666, then tab-to-search can be in the wrong spot for everyone on 87. If we back out bug 1695666, then tab-to-search is fixed but people enrolled in the QS experiment can see flicker in the QS row as they're typing under certain condition, which is annoying and distracting. If we take this patch, then both things are fixed.
• 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 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch narrows the fix to the regressing bug by applying it only to the last row in the urlbar results.

I marked Yes for the test coverage question because this code is covered by automated tests, but there's no automated test specifically for this bug because it's hard to write one.

• String changes made/needed:

Comment 12

[Pablo]

Checked on nightly 88.0a1 on Windows 10 64bit, Ubuntu 20 64bit and macOs 10.15 and the position is now #2.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9208504 [details]
Bug 1697517 - Apply suggestedIndex reuse logic only to the final row in the view to avoid breaking tab to search.

Given bug 1698496 I'll back out bug 1695666 instead for 87.

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/7b73c14202a3

Comment 15

[Drew Willcoxon :adw]

Just FTR, bug 1698496 is not surprising exactly because this bug narrowed the fix to bug 1695666. In other words, by backing out bug 1695666 instead of taking this fix, we've gone from flickering happening in some cases to flickering happening in more cases.

Comment 16

[Drew Willcoxon :adw]

Attachment: Bug 1697517 - Improve the suggestedIndex-row-reuse logic in the urlbar.

For now this only improves the earlier quick fix from D108083. We should be
checking queryContext.maxResults instead of this._rows.children.length due
to the possible presence of stale hidden rows after all the visible rows.

Ultimately we should fix this for all rows, and I'd like to do that in this bug,
but the next merge is next week and there just isn't time. Right now we should
prioritize fixing this for the problem at hand, flickering in the quick suggest
result.

Comment 17

[Phabricator Automation]

Comment on attachment 9209276 [details]
Bug 1697517 - Improve the suggestedIndex-row-reuse logic in the urlbar.

Revision D108519 was moved to bug 1698496. Setting attachment 9209276 [details] to obsolete.

Comment 18

[Pablo]

verified on windows 10, MacOS 10.15 and Ubuntu 20 with FX 87.0 build2 , Build ID 20210315170302
the position always shows #2 now.