1936557 - Unified Search Button flickers when opening/restarting Firefox

Status: ASSIGNED

(Firefox :: Address Bar, defect, P3)

Description

[Dana Lucaci, Desktop QA]

Attachment: Untitled design.mp4

Notes

• Does not reproduce with browser.urlbar.usb.dynamic = false

Found in

• Nightly 135.0a1;

Affected versions

• Nightly 135.0a1;

Affected platforms

• Windows 10/11;
• Ubuntu 22;
• macOS 14;

Preconditions

• browser.urlbar.usb.dynamic set to true
• browser.urlbar.scotchBonnet.override set to true(default)

Steps to reproduce
1. Launch Firefox and observe the Unified Search button.

Expected result

• No flickering when transitioning from magnifying icon to default search engine icon.

Actual result

• Brief flickering when transitioning from magnifying icon to default search engine icon.

Regression range

• Will look for a regression asap.

Comment 1

[Florian Quèze [:florian]]

I don't think S4/P3 is correct for a regression that had the side effect of causing the entire startup and window opening flicker tests to be disabled.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: WIP: Bug 1936557: Get default search engine information in build and use it on startup

Comment 3

[Daisuke Akatsuka (:daisuke)]

I tried to load icon data on booting, it seems that as long as we load the icon from file asynchronously, flickering happens no matter where we do, as far as I did. And it seems that the flickering failure on test was caused by not only icon but placeholder.
So, I am trying another approach below.

1. When building, load default search engine information from the config file and write out JS to use them.
2. Call the JS on init the browser to set the search engine name and icon without async.
3. If user changes the default search engine, save the name and the icon in preferences.

I tried to make a first prototype. (I didn't implement 3, also still there are many things to do though..)
As far as looking at the try, could avoid the flickering.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=Y5kH3p-BQjGBXtKGYEGn_w.1&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=cc2f8fb026b06877c2308384a43c75648e1d04d9

But, I want to confirm whether this approach works or not.
Dale, Mark, when you have time, could you please check the patch?
https://phabricator.services.mozilla.com/D237309
(I will continue to work with assuming this works)
Thanks!

Comment 4

[Mark Banner (:standard8)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

I tried to load icon data on booting, it seems that as long as we load the icon from file asynchronously, flickering happens no matter where we do, as far as I did. And it seems that the flickering failure on test was caused by not only icon but placeholder.

The placeholder search engine is currently cached in a preference which is read synchronously startup, so in theory that shouldn't be flickering unless something has gone wrong in that code. I see the test mentions that it is known that it moves by a few pixels, maybe that is what you are seeing?

1. When building, load default search engine information from the config file and write out JS to use them.
2. Call the JS on init the browser to set the search engine name and icon without async.
3. If user changes the default search engine, save the name and the icon in preferences.

Whilst this is an interesting idea, I think the problem is that we can't determine the default search engine at build time. There's too many factors that go into what is the default engine to determine that way (e.g. region, distributions, enterprise policies) so we're likely to get it wrong in a lot of cases (especially if things changed in future).

I think we have a couple of options:

• Don't display any icon until the SS has finished starting up, i.e. we don't even display the magnifying glass fallback icon until we know.
  • I don't know if this will still count as "flicker" though. We might need to talk to someone on the frontend team about this (mconley maybe), and maybe check with UX.
• See if there's anything in the skeleton UI that we could build on. I know we don't have it on all platforms, but maybe there's some ideas that will help.
• Use the fallback magnifying glass icon on first startup, and only change once the user switches tabs or loads a page.
  • This is how the current placeholder string works.
  • The downside is that we're going to be storing a potentially large icon string in preferences, which I'd really like to avoid as that's a potential performance issue.

Comment 5

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #4)

• Don't display any icon until the SS has finished starting up, i.e. we don't even display the magnifying glass fallback icon until we know.
  • I don't know if this will still count as "flicker" though. We might need to talk to someone on the frontend team about this (mconley maybe), and maybe check with UX.

It still counts as flicker, but is not as bad, and might be acceptable, if the alternatives are too expensive. The worst kinds of flicker are displaying something in the wrong position and moving it, or displaying something briefly before replacing it. In those 2 cases, even if it is very short, it is user perceptible. On the other hand, if something appears slightly later in an empty area that was previously reserved, users won't notice, so that kind of flicker will only be user-visible for users experiencing very slow startups or window opening times (in which case the actual performance is more worrying than the perceived performance).

Comment 6

[Mark Banner (:standard8)]

The team just discussed this some more, and we think delaying the display of the icon on startup is acceptable for now at least. The preference option could be possible, if we also rescale the icons to no more than 16x16 - though then we'd want to use the full scale at some time.

Comment 7

[Daisuke Akatsuka (:daisuke)]

(In reply to Mark Banner (:standard8) from comment #6)

The team just discussed this some more, and we think delaying the display of the icon on startup is acceptable for now at least. The preference option could be possible, if we also rescale the icons to no more than 16x16 - though then we'd want to use the full scale at some time.

Thank you very much, Mark!
Okay, I will try it! So, there are several possible update timings.

1. Wait until finishing the booting sequence, then update automatically.
2. As you suggested in comment 4, use magnifying glass icon on first startup, then change the icon once the user switches tabs or loads a page.
3. Keep the magnifying glass icon until user changes the search engine.

For all other cases except the actual first booting, if we can save the engine name and icon as data url in prefs, the flickering can be likely fixed if we set it in onBeforeInitialXULLayout() in browser-init, like I did in the prototype patch. If we can do so, I will choose option 1.
I will try the above next, but if you have better idea, please let me know!

Comment 8

[Mark Banner (:standard8)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #7)

For all other cases except the actual first booting, if we can save the engine name and icon as data url in prefs, the flickering can be likely fixed if we set it in onBeforeInitialXULLayout() in browser-init, like I did in the prototype patch. If we can do so, I will choose option 1.

Hmm, the existing placeholder content is set in onDOMContentLoaded, my reading of the code suggests that should be early enough to avoid flicker?

Comment 9

[Daisuke Akatsuka (:daisuke)]

(In reply to Mark Banner (:standard8) from comment #8)

(In reply to Daisuke Akatsuka (:daisuke) from comment #7)

For all other cases except the actual first booting, if we can save the engine name and icon as data url in prefs, the flickering can be likely fixed if we set it in onBeforeInitialXULLayout() in browser-init, like I did in the prototype patch. If we can do so, I will choose option 1.

Hmm, the existing placeholder content is set in onDOMContentLoaded, my reading of the code suggests that should be early enough to avoid flicker?

Thanks, Mark!
Yes, initPlaceHolder is called when onDOMContentLoaded.
But, when booting the first time, as browser.urlbar.placeholderName was not set yet, we show the default placeholder defined in html [1].
This placeholder is Search or enter address [2].
Then, we update the placeholder using the default search engine in delayedStartupInit() [3].
The placeholder is like Search with Google or enter address [4]
It seems that this change was judged as flickering.
So, as with the icon, we likely need to care it as well.

[1] https://searchfox.org/mozilla-central/rev/7c573d9eb97e7b1ba383239bfac9260b26983544/browser/base/content/navigator-toolbox.inc.xhtml#340
[2] https://searchfox.org/mozilla-central/rev/7c573d9eb97e7b1ba383239bfac9260b26983544/browser/locales/en-US/browser/browser.ftl#545-546
[3] https://searchfox.org/mozilla-central/rev/7c573d9eb97e7b1ba383239bfac9260b26983544/browser/components/urlbar/UrlbarInput.sys.mjs#3805-3809
[4] https://searchfox.org/mozilla-central/rev/7c573d9eb97e7b1ba383239bfac9260b26983544/browser/locales/en-US/browser/browser.ftl#588-589