1394914 - sidebar-button image is loaded after some delay, causing it to go from blank to an icon

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: toolbar-flicker.mov

See attached video.

Interestingly, that seems to happen in sync with the content of the new tab page being painted

Comment 1

[Florian Quèze [:florian]]

I can't reproduce in today's nightly, is it possible that you tested in a build before I fixed bug 1373331?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

No, I had tested with the most recent Nightly. I also wondered if this was a regression, and went back to find it, but it turns out it happens on any build with the screenshots button.

I just re-tested with the latest nightly. The screenshots button has been removed (moved to the Page Actions menu), but this still happens for the Sidebars button.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

I believe this could be a  _introduceNewBuiltinWidgets problem, since it appears to be profile-dependent (as it doesn't reproduce for Florian).  I have a lot of crap in browser.uiCustomization.state, which I'm attaching here. I toggled browser.uiCustomization.debug to true, but didn't see any logging in the Browser Console, although I see an error which I'm not sure if it's related or not, but looks suspicious: in toolbar.xml:145 (self._init is not a function)


I imagine (just guessing here) that the root cause for the sidebar-button and the screenshots button could be different (the screenshots one being a problem if it being not built-in, but coming from an add-on.). The Screenshots one is less of a problem for a default profile now, since it was moved to the Page Actions menu, but I think that should also be looked into (perhaps in a separate bug), if the same happens for other addons that adds toolbar buttons.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: browser-uicustomization-state.txt

Note: even though it appears there's a lot of historical crap here, my toolbar is not full of buttons.. It looks like a standard toolbar, except for that fact that the spacers are smaller.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: My toolbar.png

What my toolbar looks like, in case it's useful to look at this and the .json together

Comment 6

[Florian Quèze [:florian]]

I still can't reproduce, neither on my mac with my normal profile, nor with a new profile (even with your browser.uiCustomization.state values), nor on the reference hardware.

Comment 7

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> I still can't reproduce, neither on my mac with my normal profile, nor with
> a new profile (even with your browser.uiCustomization.state values), nor on
> the reference hardware.

Are you actually looking at a screencast, or just using your eyes? Because I could reproduce issues here when looking at a screencast, all the way back to January, see screencasts in bug 1374224, even when I couldn't *see* them myself, because the number of frames where we display intermediate state is so low.

Comment 8

[:Gijs (he/him)]

FWIW, it's not clear to me this isn't just a dupe of bug 1374224, though I guess in theory if CustomizableUI initializes late enough, an API rather than XUL-based widget like the sidebar button might in principle be not present in the markup until "later" -- but CUI should be initialized for readystatechange=complete, which I would expect to fire well before firstpaint based on what we measured/did during Australis.

Comment 9

[Florian Quèze [:florian]]

(In reply to :Gijs (queue backed up, slow) from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > I still can't reproduce, neither on my mac with my normal profile, nor with
> > a new profile (even with your browser.uiCustomization.state values), nor on
> > the reference hardware.
> 
> Are you actually looking at a screencast, or just using your eyes? Because I
> could reproduce issues here when looking at a screencast, all the way back
> to January, see screencasts in bug 1374224, even when I couldn't *see* them
> myself, because the number of frames where we display intermediate state is
> so low.

Actually, I can reproduce on the reference hardware, but only at startup, not for new windows. And indeed, only on a screen recording. Is it possible that the sidebar icon doesn't have its width set by default, and needs a CSS fix like I did in https://hg.mozilla.org/integration/mozilla-inbound/rev/2a14d2f83f51 for webextension icons? And yeah, maybe this is just bug 1374224.

Comment 10

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to :Gijs (queue backed up, slow) from comment #7)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > > I still can't reproduce, neither on my mac with my normal profile, nor with
> > > a new profile (even with your browser.uiCustomization.state values), nor on
> > > the reference hardware.
> > 
> > Are you actually looking at a screencast, or just using your eyes? Because I
> > could reproduce issues here when looking at a screencast, all the way back
> > to January, see screencasts in bug 1374224, even when I couldn't *see* them
> > myself, because the number of frames where we display intermediate state is
> > so low.
> 
> Actually, I can reproduce on the reference hardware, but only at startup,
> not for new windows. And indeed, only on a screen recording. Is it possible
> that the sidebar icon doesn't have its width set by default, and needs a CSS
> fix like I did in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2a14d2f83f51 for
> webextension icons? And yeah, maybe this is just bug 1374224.

I think none of the builtin icons have a width set (so including the sidebar button), because we expect their icons to load before we try to paint them. I don't know how easy it is to change that, it depends on the padding setup etc., which has changed a bit in recent months and I haven't kept on top of it. Dão would know better than me.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Opening 2 windows, with dao's patch from bug 1397285.mov

I just tested this with Dão's patch from bug 1397285 and it worked exactly as you thought so. The button is there and the width is set, so things don't shift. But the image continues to be loaded asynchronously.  I'm attaching a screencast here.

Note: it's hard to reproduce this with the computer idle, on a clean profile. I can see this much more easily with either of:

- with my old and heavily used profile, or
- while compiling Firefox


I'll leave up to someone else to decide if this is a dupe of bug 1397285, or if we should investigate why this image is being loaded asynchronously. At least for windows that are not the first one, I believe this image should already be preloaded like the other ones are, right?

Comment 12

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #11)

> I'll leave up to someone else to decide if this is a dupe of bug 1397285, or
> if we should investigate why this image is being loaded asynchronously.

We should investigate and fix.

Comment 13

[Florian Quèze [:florian]]

Attachment: Tentative patch

This seems to fix it for me locally on Mac (I haven't pushed this to try yet to verify if it helps on other platforms too).

Requesting only feedback for now because I really don't know if overlays are already loaded at this point / if this will regress bug 554279 (but the test_bug554279.xul and test_toolbar.xul tests touched by bug 554279 still pass locally with the patch applied).

Comment 14

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #13)
> I really don't know if overlays are
> already loaded at this point

If the overlays this comment in the code talks about were introduced by legacy xul add-ons, we may now be able to just remove that delay completely.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8937366 [details] [diff] [review]
Tentative patch

Review of attachment 8937366 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/toolbar.xml
@@ +28,5 @@
>            if (document.readyState == "complete") {
>              this._init();
>            } else {
>              // Need to wait until XUL overlays are loaded. See bug 554279.
> +            document.addEventListener("DOMContentLoaded", () => {

I think this is fine in terms of the concerns you raised. More generally, with the demise of XPCOM/XUL add-ons, there are no more overlay-added toolbar buttons, which is the issue here.

As a result, I actually suspect this can be simplified/fixed even further.

However, as written, AIUI readyState == "complete" happens *after* DOMContentLoaded, so isn't it possible for us to take the `else` branch here and then never hit the event because it's already happened?

Comment 16

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #15)

Thanks for the quick feedback!

> More generally,
> with the demise of XPCOM/XUL add-ons, there are no more overlay-added
> toolbar buttons, which is the issue here.
> 
> As a result, I actually suspect this can be simplified/fixed even further.

Yeah, after sleeping on it I was already going in this direction (comment 14).

> However, as written, AIUI readyState == "complete" happens *after*
> DOMContentLoaded, so isn't it possible for us to take the `else` branch here
> and then never hit the event because it's already happened?

In my local testing, things always happened in this order:
- toolbar.xml constructor
- DOMContentLoaded event
- document.readyState == "complete"
- load event

I assume 'document.readyState == "complete"' happens right before the load event, otherwise the sidebar image would (at least sometimes) have time to load asynchronously before first paint.

I think for toolbars that are part of the initial xul markup, the binding will always be loaded before the DOMContentLoaded event is fired. If toolbars are ever added dynamically by JS code running off the DOMContentLoaded event, then attachment 8937366 [details] [diff] [review] would break I guess. In any case, I agree there's some fragility here.

So... let's see if try is happy with me removing this hack completely: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ed888a5076933c6883c3018f79e48daa9e8376

Comment 17

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #16)
> So... let's see if try is happy with me removing this hack completely:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=90ed888a5076933c6883c3018f79e48daa9e8376

FWIW, I *expect* (but am obviously not 100% sure, the delay was added a long time ago and things may have changed) that this is fine. What I'd be more worried about is some talos tests regressing. IIRC this change was part of delaying toolbar initialization work to avoid delaying tpaint/ts_paint because when we worked on Australis, part of the requirements was not regressing any of those tests even a tiny bit. YMMV as to how important it is at this point.

Comment 18

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #17)
> What I'd be more
> worried about is some talos tests regressing.

I'm also slightly worried about Talos here... although this code already runs before first paint, so I'm not sure any (non-broken) talos test would have a good reason to see a regression.

Comment 19

[Florian Quèze [:florian]]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6aba7f3c81c6d409ecf4dd0a82984b69c25fc7

Comment 20

[Florian Quèze [:florian]]

The browser_windowopen_reflows.js test now fails only on Windows :-(.

Comment 21

[Florian Quèze [:florian]]

Attachment: Patch v3

Let's just add the 'new' reflow to the whitelist, I'll file a follow-up to investigate if we can disable the refresh driver to avoid the spurious overflow events generated when the window has a 1x1px size.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eccc8a860a8ef7a0f4227809985dad3e86d2c7d looks green, except for a linter error I fixed before attaching the patch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c96cab6821a
toolbars no longer need to wait for xul overlays to load before initializing CustomizableUI icons (avoid sidebar icon flickering), r=Gijs.

Comment 23

[Eliza Balazs [:ebalazs_]]

Backed out changeset 7c96cab6821a (bug 1394914) for failing bc in browser/base/content/test/performance/browser_windowopen_reflows.js r=backout a=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0a2ee6eb7ca235e25e82ac2a7dc0d4e6be6a0b55&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=26927d4b8975345b805f942d5a569fe256264265&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=152585940

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2ee6eb7ca235e25e82ac2a7dc0d4e6be6a0b55

Comment 24

[Eliza Balazs [:ebalazs_]]

Backed out changeset 7c96cab6821a (bug 1394914) for failing bc in browser/base/content/test/performance/browser_windowopen_reflows.js r=backout a=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a235bf4868ab9e48c7b2f4bf4cc9bd949ca23c35&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=152573538

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=cdc112ad2452d71d801ea7a779b194d945901a47&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

https://hg.mozilla.org/mozilla-central/rev/cdc112ad2452d71d801ea7a779b194d945901a47

Comment 25

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7c96cab6821a

Comment 26

[:Gijs (he/him)]

This clashed with bug 1380465 which landed on autoland, which is why it went orange on merge.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cff7451155d
toolbars no longer need to wait for xul overlays to load before initializing CustomizableUI icons (avoid sidebar icon flickering), r=Gijs.

Comment 28

[Florian Quèze [:florian]]

I moved the new whitelist entry to also be on Linux, and that's green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d07ca0d2c6619ad03c238dc3401a291e50bf5ae

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6cff7451155d