1593964 - Retained results on tab switch

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P2)

Description

[Marco Bonardo [:mak]]

We should retain results on tab switch, so that if the user switches tab to check or copy something, they can still return to the previous search state.
The reason we currently clear results is that on a different tab or window the user may change options, or remove history, or other operations that would make the previous results invalid.

One possible solution may be to retain results for a certain amount of time, though this would be inconsistent and confusing for the user: why are they retained in some cases and not others?

Another possible solution may be to actually requery, that is what we were initially doing. The problem with this was flicker of results on requery. There are possible solutions to this flicker problem:

1. don't clear results on tab switch, but requery. Because there are previous results, the likelyhood of a requery returning the same set is high, the flicker should be limited because we reuse results.
2. run a new query in background, update the view on a timer. May still flicker if some of the results are delayed more than usual.
3. run a new query in background, open the view when the query is complete. Some queries may take a long time to complete, especially if add-ons are involved.

Off-hand it sounds like solution 1 may do, we should try it and see how it behaves.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1593964 - part 1 - Stop storing autofilled text in userTypedValue. r=Dao

The original fix for Bug 287996 intended to store the url as userTypedValue when
it was confirmed in the UI and about to start loading, but it threw the baby out
with the bathwater. By storing any selection-completed and autofilled value as
userTypedValue, it made impossible to properly return to the typed text.
Currently, if autofill happens, switching back to the tab where it happened
shows the autofilled value instead of what the user typed.
This also makes impossible to reopen the view, because the search string changed.
Because now we properly set userTypedValue in _loadURL, we can remove this old
workaround.
Note though that this breaks selecting a url (without confirmation), switching tab,
then going back to the original tab and expecting the url to be set.
While it's possible to support that case, we think it's more important to go
back to what was typed, and eventually reopen the view. It's also less confusing
for the user, because the shown url has no relation with the currently loaded page.

For similar reasons, blurring the urlbar should restore the user typed value
if we autofilled a value, to avoid overriding the search string with our own.
And because of this, we should not clear the view every time set a value, or we
could clear valid cached results. It's better to do it on an actual input event.

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1593964 - part 2 - Retain results on tab switch/deactivate, requery on reopen. r=dao

Rather than clearing results on tab switch or deactivate, let's keep them, and
clean them on input change. On reopening the view run the query again, so results
are up to date and autofill works, but flickering is reduced.

Depends on D52644

Comment 3

[Harry Twyford]

From this Phabricator comment for bug 1593964:

failures stem from how gURLBar.search("") currently hangs when the megabar is enabled. This is fixed by @mak's changes in bug 1593964. I thought it would just make sense to wait for that to land rather than porting a bunch of stuff over from that patch, seeing as it seems pretty close.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b744d1bf8e7
part 1 - Stop storing autofilled text in userTypedValue. r=dao
https://hg.mozilla.org/integration/autoland/rev/7f2fe5517b7e
part 2 - Retain results on tab switch/deactivate, requery on reopen. r=dao

Comment 5

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing failures in browser_retainedResultsOnFocus.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/e70b2faff1dd8a48a1d22c29f55901b0f50e2712

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=598800&selectedJob=279293661&resultStatus=testfailed%2Cbusted%2Cexception&tochange=e70b2faff1dd8a48a1d22c29f55901b0f50e2712&fromchange=7f2fe5517b7e733e562e2ce69bff536b7bc904d8&searchStr=mochitest

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=279293661&repo=autoland&lineNumber=7545

[task 2019-12-03T11:55:04.473Z] 11:55:04 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_retainedResultsOnFocus.js | Test timed out -
[task 2019-12-03T11:55:04.474Z] 11:55:04 INFO - GECKO(2032) | [Parent 2032, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0x7fbdfc6f32e0 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 108
[task 2019-12-03T11:55:04.481Z] 11:55:04 INFO - GECKO(2032) | [Parent 2032, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0x7fbdfc6f32e0 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 108
[task 2019-12-03T11:55:04.481Z] 11:55:04 INFO - GECKO(2032) | MEMORY STAT | vsize 3400MB | residentFast 475MB | heapAllocated 171MB
[task 2019-12-03T11:55:04.482Z] 11:55:04 INFO - TEST-OK | browser/components/urlbar/tests/browser/browser_retainedResultsOnFocus.js | took 90255ms
[task 2019-12-03T11:55:04.483Z] 11:55:04 INFO - GECKO(2032) | [Child 2243: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7fa1767dd000 == 1 [pid = 2243] [id = {55e7b94f-b480-4464-8b86-de1bb118af43}]
[task 2019-12-03T11:55:04.483Z] 11:55:04 INFO - GECKO(2032) | [Child 2243: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x7fa17aaaff20) [pid = 2243] [serial = 32] [outer = (nil)]
[task 2019-12-03T11:55:04.484Z] 11:55:04 INFO - GECKO(2032) | [Child 2243: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x7fa176756000) [pid = 2243] [serial = 33] [outer = 0x7fa17aaaff20]
[task 2019-12-03T11:55:04.485Z] 11:55:04 INFO - checking window state
[task 2019-12-03T11:55:04.486Z] 11:55:04 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-12-03T11:55:04.486Z] 11:55:04 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_retainedResultsOnFocus.js | Found a browser window after previous test timed out -
[task 2019-12-03T11:55:04.487Z] 11:55:04 INFO - GECKO(2032) | must wait for focus
[task 2019-12-03T11:55:04.488Z] 11:55:04 INFO - GECKO(2032) | [Parent 2032, Main Thread] WARNING: NS_ENSURE_TRUE(GetWrapper()) failed: file /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp, line 55
[task 2019-12-03T11:55:04.489Z] 11:55:04 INFO - GECKO(2032) | [Parent 2032, Main Thread] WARNING: '!inner', file /builds/worker/workspace/build/src/dom/ipc/JSWindowActorService.cpp, line 181
[task 2019-12-03T11:55:04.489Z] 11:55:04 INFO - TEST-START | browser/components/urlbar/tests/browser/browser_searchTelemetry.js

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1593964 - part 3 - Fix the urlbar event bufferer handling of the initial query status. r=dao

The event bufferer has 2 problems:

1. startDate is initialized to null and then compared to Cu.now(), that generates unexpected results
2. when the initial status is UNKNOWN events should not be deferred (down in a new window for example)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/520b534bb543
part 1 - Stop storing autofilled text in userTypedValue. r=dao
https://hg.mozilla.org/integration/autoland/rev/0688bebca797
part 2 - Retain results on tab switch/deactivate, requery on reopen. r=dao
https://hg.mozilla.org/integration/autoland/rev/7c234b6314b3
part 3 - Fix the urlbar event bufferer handling of the initial query status. r=harry

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/520b534bb543
https://hg.mozilla.org/mozilla-central/rev/0688bebca797
https://hg.mozilla.org/mozilla-central/rev/7c234b6314b3

Comment 9

[gwarser]

Isn't this reintroducing this "security issue" https://bugzilla.mozilla.org/show_bug.cgi?id=724599 ?

Instead of drag&drop, now it's copy&paste?

STR:

Copy https://google.com and paste into URL bar on this page, then remove focus from URL bar

This bug probably fixes issue I'm experiencing, but I was told potential fix can cause security problems: https://bugzilla.mozilla.org/show_bug.cgi?id=610357#c144

Comment 10

[gwarser]

Its' you Marco Bonardo who told me this, I'm confused.

Comment 11

[gwarser]

Ah, ok, so this bug is about results in drop down list. But I'm still confused - why URL bar content is allowed to stay when pasted or typed, but not on d&d. It's not easy to distinguish if it's real page address or modified because both padlock and magnifier icons are grey.