Closed
Bug 1394914
Opened 7 years ago
Closed 7 years ago
sidebar-button image is loaded after some delay, causing it to go from blank to an icon
Categories
(Firefox :: Toolbars and Customization, defect, P3)
Tracking
()
RESOLVED
FIXED
Firefox 59
People
(Reporter: Felipe, Assigned: florian)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(6 files, 1 obsolete file)
See attached video.
Interestingly, that seems to happen in sync with the content of the new tab page being painted
Assignee | ||
Comment 1•7 years ago
|
||
I can't reproduce in today's nightly, is it possible that you tested in a build before I fixed bug 1373331?
Reporter | ||
Comment 2•7 years ago
|
||
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.
Summary: Screenshots and Sidebar icons load asynchronously, causing entire toolbar to flicker and shift when opening → Sidebars toolbar button loads asynchronously, causing the entire toolbar to flicker and shift when opening a new window
Reporter | ||
Comment 3•7 years ago
|
||
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.
Summary: Sidebars toolbar button loads asynchronously, causing the entire toolbar to flicker and shift when opening a new window → sidebar-button is added asynchronously, causing the entire toolbar to flicker and shift when opening a new window
Reporter | ||
Comment 4•7 years ago
|
||
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.
Reporter | ||
Comment 5•7 years ago
|
||
What my toolbar looks like, in case it's useful to look at this and the .json together
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
(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.
Reporter | ||
Comment 11•7 years ago
|
||
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?
Updated•7 years ago
|
Priority: -- → P5
Summary: sidebar-button is added asynchronously, causing the entire toolbar to flicker and shift when opening a new window → sidebar-button image is loaded after some delay, causing it to go from blank to an icon
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Priority: P5 → P3
Assignee | ||
Comment 13•7 years ago
|
||
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).
Attachment #8937366 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 14•7 years ago
|
||
(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•7 years ago
|
||
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?
Attachment #8937366 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 16•7 years ago
|
||
(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•7 years ago
|
||
(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.
Assignee | ||
Comment 18•7 years ago
|
||
(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.
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8937456 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 20•7 years ago
|
||
The browser_windowopen_reflows.js test now fails only on Windows :-(.
Assignee | ||
Comment 21•7 years ago
|
||
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.
Attachment #8937555 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8937456 -
Attachment is obsolete: true
Attachment #8937456 -
Flags: review?(gijskruitbosch+bugs)
Updated•7 years ago
|
Attachment #8937555 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 22•7 years ago
|
||
Pushed by florian@queze.net:
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Comment 26•7 years ago
|
||
This clashed with bug 1380465 which landed on autoland, which is why it went orange on merge.
Comment 27•7 years ago
|
||
Pushed by florian@queze.net:
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.
Assignee | ||
Comment 28•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in
before you can comment on or make changes to this bug.
Description
•