Closed Bug 1434945 Opened 7 years ago Closed 7 years ago

browser.xul should look correct if it's painted after domcontentloaded

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [fxperf])

Attachments

(4 files, 3 obsolete files)

part 3 - CSS sizes
(deleted), patch
johannh
: review+
Details | Diff | Splinter Review
part 1 - UI density v2
(deleted), patch
Details | Diff | Splinter Review
part 2 - tabbar visibility v3
(deleted), patch
Details | Diff | Splinter Review
part 4 toolbar.xml should ignore unrelated underflow events
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
When creating a new xul window containing browser.xul, the window becomes visible at the end of the 'load' event, at which point we already have all SVG icons loaded. When instead of creating a new window to load browser.xul, we change the location of an about:blank window to browser.xul (what I'm planning to do in bug 1336227), things are messed up in the first-paint layout, causing very noticeable flicker. After investigating, there are actually only 3 things that need fixing: - the tab bar needs to become visible in a 'domcontentloaded' rather than a 'load' event listener. - the UI density needs to be set in a 'domcontentloaded' listener rather than a 'load' listener. - some SVG icons have their width set with CSS but not their height (or the height but not their width).
Attached patch part 1 - tabbar visibility (obsolete) (deleted) — — Splinter Review
Attachment #8947503 - Flags: review?(jhofmann)
Attached patch part 2 - UI density (obsolete) (deleted) — — Splinter Review
Attachment #8947504 - Flags: review?(jhofmann)
Attached patch part 3 - CSS sizes (deleted) — — Splinter Review
Attachment #8947505 - Flags: review?(jhofmann)
Priority: -- → P1
Whiteboard: [fxperf]
Attachment #8947503 - Flags: review?(jhofmann) → review+
Attachment #8947505 - Flags: review?(jhofmann) → review+
Comment on attachment 8947504 [details] [diff] [review] part 2 - UI density Review of attachment 8947504 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8947504 - Flags: review?(jhofmann) → review+
Attached patch part 1 - tabbar visibility v2 (obsolete) (deleted) — — Splinter Review
Turns out this removes some sync reflows when opening windows! \o/
Attachment #8947503 - Attachment is obsolete: true
Attached patch part 1 - UI density v2 (deleted) — — Splinter Review
Turns out this was leaking on the toolkit/components/startup/tests/browser/browser_bug537449.js test that closes a browser window after domcontentloaded but before onload. Fixed by moving the ui density uninitialization before the this._loadHandled check in onUnload.
Attachment #8947504 - Attachment is obsolete: true
Attached patch part 2 - tabbar visibility v3 (deleted) — — Splinter Review
Same problem and fix.
Attachment #8948351 - Attachment is obsolete: true
My patch here setting the tabbar visibility in domcontentloaded causes lots of browser/components/customizableui test failures https://treeherder.mozilla.org/#/jobs?repo=try&revision=49601d1bfc1a343fb847bce45e8298b114890a02 in small windows that don't get the overflow (">>") icon in the navigation toolbar when they should. I managed to reproduce locally on my Linux machine and I observed that the overflowedDuringConstruction field gets cleared by an underflow event from the identity-box. On builds without my other patch, this underflow event also exists, but arrives after https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/browser/components/customizableui/CustomizableUI.jsm#4221 runs. So it seems to just be a timing issue. Given that onOverflow already filters the unrelated events at https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/browser/components/customizableui/CustomizableUI.jsm#4347 I think it's fine to also filter underflow events in the same way. I'm still waiting for new try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=02964256cbbb3983c9c787ac73fddc541dc37968
Attachment #8949369 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8949369 [details] [diff] [review] part 4 toolbar.xml should ignore unrelated underflow events Review of attachment 8949369 [details] [diff] [review]: ----------------------------------------------------------------- r=me, can you file a follow-up bug to move all this logic into CUI or some other JSM, out of the binding? For one it's XBL (which we're trying to remove) for another it's duplicated so it's hard to maintain this way (as evidenced by this patch duplicating logic we have elsewhere). ::: browser/components/customizableui/content/toolbar.xml @@ +49,5 @@ > > <method name="handleEvent"> > <parameter name="aEvent"/> > <body><![CDATA[ > + // Ignore overflow/underflow events from the identity-box. I would just say "from nodes inside the toolbar"
Attachment #8949369 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/92be7d743027 the UI density should be set from a DOMContentLoaded listener, r=johannh. https://hg.mozilla.org/integration/mozilla-inbound/rev/0bac3c3269f1 the tabbar visibility should be set during a DOMContentLoaded listener, r=johannh. https://hg.mozilla.org/integration/mozilla-inbound/rev/3145f2c95016 toolbar.xml should ignore overflow/underflow events that don't come from its customization target, r=Gijs. https://hg.mozilla.org/integration/mozilla-inbound/rev/23cd8f9962b5 icons of the main browser UI should have both their height and width set in CSS to avoid flickering when the SVG icons load, r=johannh.
Depends on: 1438490
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: