(Copied from https://github.com/mozilla-services/screenshots/issues/3026) ----------------------------------------------- See this video recording of Firefox startup captured on the quantum reference hardware: https://bug1372518.bmoattachments.org/attachment.cgi?id=8877069 At first paint there's no space reserved in the toolbar for the screenshot icon. Then some empty area appears. Then this area is expended and the screenshot icon appears in it. Then the screenshot icon disappears. Then it's replaced by an icon with a purple star. Ideally the correct icon should already be displayed at first paint. If that turns out to be impossible, we should at least ensure the right amount of space is reserved at first paint, so that we don't see the location bar and searchbar shifting to the left twice (it actually shifts a third time on this video, but that's either due to Pocket or to the profiler add-on). If I could file this on bugzilla, it would block the photon-startup bug. -----------------------------------------------

(Comment from Ian on the original report): I believe this happens because we delay the WebExtension startup, in an effort to keep Screenshots from affecting overall Firefox performance. The button is itself implemented as part of the WebExtension, and so the space is created when the embedded WebExtension is first started, and the icon appears later due to whatever resource loading process happens as part of the WebExtension system. I don't have any particular thoughts or suggestions on a fix.

Kris -- ideas on this?

Most of the problem should be fixed by setting a minimum size for the button image, so it doesn't resize after the image finishes loading.

So I updated it so the icon container size doesn't change, which looks better but there's still one frame where there's no icon between it initially loading and the blue * badge being added. If you go through the onboarding for screenshots then the badge isn't added and the flicker goes away. The profiler icon does the same thing in the original video when it switches from the default grey icon to the enabled blue icon.

This does not appear to be specific to load time. When navigating to a page with React on it the React devtools will update the icon. In this case it takes quite a few frames for the icon to appear and it is blank during the transition.

This flicker isn't specific to startup, it also happens for each new window.

The flicker of the screenshot icon when opening a new window is due to the requestAnimationFrame call at: http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/components/extensions/ext-browserAction.js#434 Removing it avoids the flicker, but using rAF made sense for already loaded windows.

This is a simple (and ugly) hack avoiding the rAF call when we run before first paint.

Is there a reason why we even need to update the dom of these toolbar buttons for every tab switch? For icons like the gecko profiler or the screenshot icon, this seems wasteful.

From my testing the flicker happens most times the icon changes. It appeared to me that it was because the icon is set with CSS using `list-style-image` and that will remove the old image until the new image loads and replaces it. I have a patch that preloads these images before updating the CSS but it is definitely not efficient (it loads all possible icons currently). I'm not sure if this is the best approach, if we can identify which image to preload it might work fine. Ideally improving when we are updating the DOM for those buttons too. I tried your patch and I still see a flicker on my Mac when I record a video and step through the frames.

(In reply to Florian Quèze [:florian] [:flo] from comment #9) > Is there a reason why we even need to update the dom of these toolbar > buttons for every tab switch? For icons like the gecko profiler or the > screenshot icon, this seems wasteful. The API allows for tab-specific icons, so we need to check for changes on tab switch. Theoretically we could optimize for the case where there aren't any tab-specific icons, but the generated CSS value is cached, so I'd be surprised if it made a measurable difference.

Comment on attachment 8895978 [details] [diff] [review] Simple hack Review of attachment 8895978 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-browserAction.js @@ +465,5 @@ > } > > node.setAttribute("style", style); > + }; > + if (!node.ownerGlobal.gBrowser._loadHandled) We should probably just pass an additional argument when this is being called from Widget.onCreated. We should always want to set the button attributes synchronously at that point.

Attachment 8898284 [details] [diff] is enough to avoid the toolbar flicker in new windows from the screenshots icon, but testing with the gecko profiler add-on I still see flickering. I wonder if that profiler icon appears later because it's a PNG file we need to decode rather than an SVG file for the screenshot icon. This simple CSS patch reserves 16px in the toolbar for the icon, to avoid having the whole toolbar shifting to the left when the icon eventually loads a couple frames later.

Comment on attachment 8898284 [details] [diff] [review] Patch Review of attachment 8898284 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-browserAction.js @@ +469,5 @@ > + }; > + if (sync) > + callback(); > + else > + node.ownerGlobal.requestAnimationFrame(callback); Nit: Braces around all if-else bodies (ESLint will enforce this)

Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/58ecca41f432 update browserAction buttons synchronously when creating them to avoid flickering in new windows, r=kmag. https://hg.mozilla.org/integration/mozilla-inbound/rev/2a14d2f83f51 reserve 16px in the toolbar for browserAction button icons to avoid flickering when the icon loads, r=kmag.

https://hg.mozilla.org/mozilla-central/rev/58ecca41f432 https://hg.mozilla.org/mozilla-central/rev/2a14d2f83f51

In this bug I handled the general browserAction action issues. For the screenshot icon specifically, we'll still have some flicker right after first paint of the first window as the screenshot initialization is delayed during startup.

Tested on: 57.0a1 20170823100553 Windows 10 x64 pro, Ubuntu 15.04 x64, Mac Osx 16.04 For new window, LGTM, but on first paint, I still see part of the bug still reproducible: - https://screenpresso.com/=Q85Jb - Windows 10 - ~ same results on Mac and Linux as well.

(In reply to Adrian Florinescu [:AdrianSV] from comment #19) > on first paint, I still see part of the bug still > reproducible: > - https://screenpresso.com/=Q85Jb - Windows 10 - ~ same results on Mac and > Linux as well. Seems like I missed that https://screenpresso.com/=Q85Jb is bug 1374216.