Replace the panel XBL bindings with Custom Elements
Categories
(Toolkit :: XUL Widgets, task, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox71 | --- | fixed |
People
(Reporter: bgrins, Assigned: surkov)
References
Details
(Whiteboard: [xbl-custom-element][rebase-needed])
Attachments
(3 files, 10 obsolete files)
XUL <panel> elements can have different bindings attached. The inheritance looks like this: popup-base (implements nsIDOMXULPopupElement) panel (implements nsIDOMXULPopupElement) arrowpanel datetime-popup * popup-base is never directly bound to an element * panel is bound to `<panel>` elements (https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/toolkit/content/xul.css#420) * arrowpanel is bound to `<panel type="arrow">` (https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/toolkit/content/xul.css#441) * datetime-popup is bound to `#DateTimePickerPanel[active="true"]` (https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/browser/base/content/browser.css#667) There are a couple of things to investigate wrt converting to Custom Elements: 1) CE are defined for a tag and can't have CSS binding as used in arrowpanel and datetime-popup. This means we either need to: a) fold the logic from those two bindings into the main panel CE b) create new tags for each binding and update references to `nsGkAtoms::panel` in platform to also look for the new tags 2) They use the [implements] feature, so we need to figure out a way to connect the js class to nsIDOMXULPopupElement
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•7 years ago
|
||
This patch is very WIP, but it appears the hamburger panel is opening and positioning itself correctly (others are broken)
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 3•6 years ago
|
||
Will be interesting to see if we have the same hang we are seeing with <wizard>, if we try to port arrowpanel to use Shadow DOM (https://bugzilla.mozilla.org/show_bug.cgi?id=1495621#c8).
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
The latest patch works almost perfectly (animation works great, opening/closing works, ...). The only broken thing atm is that the styling from other document stylesheets don't apply.
I've also triggered a try push in case I missed something:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9647426be1b96f29bd1807c06ddb8e665e53c353
Reporter | ||
Comment 6•6 years ago
|
||
This is blocked on Shadow Parts (Bug 1505489) but marking it as assigned since Tim's patch looks good besides that.
Updated•6 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Taking this out of my queue until I have more time later on to take a look at it.
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
Reporter | ||
Comment 12•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #11)
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
I talked to Tim and it's basically addressing review comments. Could you pick it up?
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #12)
(In reply to alexander :surkov (:asurkov) from comment #11)
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
I talked to Tim and it's basically addressing review comments. Could you pick it up?
do you know whether those patch are coming together or could be finished/landed separately?
Reporter | ||
Comment 14•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #13)
(In reply to Brian Grinstead [:bgrins] from comment #12)
(In reply to alexander :surkov (:asurkov) from comment #11)
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
I talked to Tim and it's basically addressing review comments. Could you pick it up?
do you know whether those patch are coming together or could be finished/landed separately?
The CSS refactor could kind of be independent, but it does rely on shadow :host stuff so it would be some extra work to split it out. It might be worth doing that way in a new bug to make it easier to track styling regressions. But in the meantime I think the review comments can be addressed as separate patches on this bug.
Assignee | ||
Comment 15•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #14)
(In reply to alexander :surkov (:asurkov) from comment #13)
(In reply to Brian Grinstead [:bgrins] from comment #12)
(In reply to alexander :surkov (:asurkov) from comment #11)
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
I talked to Tim and it's basically addressing review comments. Could you pick it up?
do you know whether those patch are coming together or could be finished/landed separately?
The CSS refactor could kind of be independent, but it does rely on shadow :host stuff so it would be some extra work to split it out. It might be worth doing that way in a new bug to make it easier to track styling regressions. But in the meantime I think the review comments can be addressed as separate patches on this bug.
does not phab allow to continue exiting patch? I guess I squashed my changes with original patch into one already
Assignee | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
Apparently neither UploadDiff nor specified Differential Revision in the patch works. Are you sure you want the changes as a separate patch?
Reporter | ||
Comment 18•5 years ago
|
||
FYI - there are also some existing test failures with the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3fa7044e0bb8908f399ee62e1ff1bde6a23f474&selectedJob=251451602.
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
Assignee | ||
Comment 21•5 years ago
|
||
Assignee | ||
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #15)
(In reply to Brian Grinstead [:bgrins] from comment #14)
(In reply to alexander :surkov (:asurkov) from comment #13)
(In reply to Brian Grinstead [:bgrins] from comment #12)
(In reply to alexander :surkov (:asurkov) from comment #11)
(In reply to Tim Nguyen :ntim from comment #10)
Taking this out of my queue until I have more time later on to take a look at it.
What is the current status? Is there anything left beyond addressing reviewer comments?
I talked to Tim and it's basically addressing review comments. Could you pick it up?
do you know whether those patch are coming together or could be finished/landed separately?
The CSS refactor could kind of be independent, but it does rely on shadow :host stuff so it would be some extra work to split it out. It might be worth doing that way in a new bug to make it easier to track styling regressions. But in the meantime I think the review comments can be addressed as separate patches on this bug.
does not phab allow to continue exiting patch? I guess I squashed my changes with original patch into one already
Yes, you can do "Commandeer revision" in "Add Action...".
Also, another thing that needs to be done is double checking the order of event handlers (whether the arrowpanel or the panel code runs first). The current patch assumes the arrowpanel event handlers run first, but that could be wrong (and make tests fail).
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 24•5 years ago
|
||
FYI, instead of obsoleting patches and creating new ones, you can take over patches from other authors using "Commandeer Revision" under "Add Action..." at the bottom of phabricator pages.
Assignee | ||
Comment 25•5 years ago
|
||
(In reply to DΓ£o Gottwald [::dao] from comment #24)
FYI, instead of obsoleting patches and creating new ones, you can take over patches from other authors using "Commandeer Revision" under "Add Action..." at the bottom of phabricator pages.
yeah, Tim mentioned that too, I just send the patches right before I was told about this option. I'm curious though, if the patch has to contain any meta information before it can be sent to phab so that it will be recognized as an existing patch, and if so, then what kind of info that should be.
Comment 26•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #25)
(In reply to DΓ£o Gottwald [::dao] from comment #24)
FYI, instead of obsoleting patches and creating new ones, you can take over patches from other authors using "Commandeer Revision" under "Add Action..." at the bottom of phabricator pages.
yeah, Tim mentioned that too, I just send the patches right before I was told about this option. I'm curious though, if the patch has to contain any meta information before it can be sent to phab so that it will be recognized as an existing patch, and if so, then what kind of info that should be.
Normally, if you put βDifferential Revision: https://...β (make sure to capitalize properly) in a new line in your commit message, it should link up.
Assignee | ||
Comment 27•5 years ago
|
||
So, on browser/base/content/test/general/browser_documentnavigation.js failure. The test opens a download panel, which is in navigation order when moving by F6. The navigation order is: urlbar (input), richlistbox inside of the download panel and content document (hosted in browser element). After the patch is applied the forward navigation order works fine, however the richlistbox is missed from navigation when moving backwards (shift+F6). Richlistbox is a child of panelmultiview element which is a slotted element under the panel custom element now. According FocusNavigation logs the panelmultiview element is no longer participating in focus navigation.
I'm curious, if Emilio has any ideas on this.
Comment 28•5 years ago
|
||
Does https://phabricator.services.mozilla.com/D35585 help by any chance? Seems like a pretty similar situation.
Updated•5 years ago
|
Comment 29•5 years ago
|
||
Comment 30•5 years ago
|
||
Comment 31•5 years ago
|
||
with attachShadow moved in constructor: https://treeherder.mozilla.org/#/jobs?repo=try&revision=483caa8b03c1edfceec579d17e191a55ff836882
Reporter | ||
Comment 32•5 years ago
|
||
Tim, this could apply onto the second commit in the stack to lower the amount of attribute inheritance.
Reporter | ||
Comment 33•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #32)
Created attachment 9091201 [details] [diff] [review]
skip-attr-inherit.patchTim, this could apply onto the second commit in the stack to lower the amount of attribute inheritance.
And perhaps the .panel-arrowcontent attributes could be done with inheriting the styles from the parent that those xul attributes set? Would need some testing. For sure "side" looks unnecessary in that list.
Reporter | ||
Comment 34•5 years ago
|
||
The only remaining test failures we see with the latest patches applied are windows specific, and seem related to the page action / browser action panels.
When you open another panel like the pocket panel or an extension browser action panel, then after closing it the page action panel is buggy. The first click on the page action panel icon doesn't open it - you have to click multiple times for it to open. Haven't tracked down the root cause yet though.
Updated•5 years ago
|
Comment 35•5 years ago
|
||
New try push with all the patches folded: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0732ed12e64794626e5ea4afbc107828bab1cf32
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 36•5 years ago
|
||
browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopLeftRadius should be the same - Got 0px, expected 3.5px
fails because stack->browser elements, which are slotted into panel element, have style applied:
.webextension-popup-browser, .webextension-popup-stack {
border-radius: inherit;
}
which inherited from the panel element, which has no border-radious styles applied. However when the panel was used to an XBL binding, then border-radious style was inherited from its insertion point, which is .panel-arrowcontent element, which has 3.5px border-radius.
Emilio, I suppose this is correct behavior, when a slotted element doesn't inherit styles from its immediate shadow DOM container?
Comment 37•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #36)
browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopLeftRadius should be the same - Got 0px, expected 3.5px
fails because stack->browser elements, which are slotted into panel element, have style applied:
.webextension-popup-browser, .webextension-popup-stack {
border-radius: inherit;
}
I believe this rule should no longer be needed if the panels are using .panel-no-padding
class (which I believe they are), since the overflow: hidden
should clip the browser to the border-radius (but I might be wrong though, feel free to double check).
Assignee | ||
Comment 38•5 years ago
|
||
Assignee | ||
Comment 39•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #37)
I believe this rule should no longer be needed if the panels are using
.panel-no-padding
class (which I believe they are), since theoverflow: hidden
should clip the browser to the border-radius (but I might be wrong though, feel free to double check).
you're right the class is applied, and overflow:hidden indeed cuts borders, however a child element having a border look visually differently depending on whether border-radius is applied or not. Originally the test was added for background-color issue (bug 1280128), so overflow:hidden should handle it nicely. Also if I understand right then border is set for arrowcontent, not for panel children, so if panel's content is never supposed to have borders, then all we need is to adjust the test.
Comment 40•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #36)
Emilio, I suppose this is correct behavior, when a slotted element doesn't inherit styles from its immediate shadow DOM container?
A slotted element inherits from the slot, not its parent. So you could have border-radius: inherit
on it too, I guess, to keep the same behavior.
Reporter | ||
Comment 41•5 years ago
|
||
Hi Emilio and Mike, we are seeing something strange with the numbers on the "twinopen" test on Linux only with this patch (transitioning from XBL to CE+SD for <panel>). I was hoping you could help us figure out what is going wrong here or give us some pointers on where to look next to help to unblock one of our last remaining XBL binding removals.
There are no issues on any other tests or platforms, but there's a 30% (!) regression on twinopen: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fe56fc003f843ffa149053eba38b5bff27ad9a82&newProject=try&newRevision=77497eec13139af964d510fee6493995a20b916f&framework=1&showOnlyImportant=1
Base push (m-c tip pushed with ./mach try fuzzy -q "linux opt-talos 'session | 'other" --gecko-profile
):
- Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64dd2a14912007055e7915e5f0fbada05769600e&selectedJob=269471481
- twinopen profile: https://perfht.ml/2n6qOEu
Same base but with the <panel> patch applied:
- Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aae1688be265a873d2fc1cc574c58fe323f4acdf&selectedJob=269471800
- twinopen profile: https://perfht.ml/2oHRonB
I've been trying out the "compare" UI in the profiler to look at these two profiles together (https://perfht.ml/2oxvwvf) and oddly I see an extra 38ms spent in gfxFcPlatformFontList::ReadSystemFontList in the base push, but perhaps that happens after the twinopen measurement is taken (which I guess is here https://searchfox.org/mozilla-central/rev/35cc00a25c4471993fdaa5761952bd3afd4f1731/testing/talos/talos/tests/twinopen/api.js#27)
Does anything jump out at you in the new profile? FYI I have already resolved the bulk of the time spent in panel.js connectedCallback in a local version (8ms in this profile) but the regression is still the same.
Comment 42•5 years ago
|
||
I'm not quite sure off-hand (I'm not familiar with this test), but I see a lot of slow style system stuff going on in places where it shouldn't, due to an old friend of ours :-)
In particular, there's a huge time spent in rebuilding the cascade data of the shadow trees when calling querySelector() from various getters from connectedCallback() and company. The issue is our old friend Element::GetBindingURL
. What kind of elements are we poking at that are causing us to resolve style?
Looking a bit closer we're querySelector'ing .panel-arrowcontent
, which is a xul:box. Not sure whether it'd be worth changing the existing check in MayNeedToLoadXBLBinding
by a blacklist? How many elements remain with XBL bindings?
That'd be probably a cheap win, though not 100% sure it explains the regression... It's most of the time in the profile looks like, so it might.
Reporter | ||
Comment 43•5 years ago
|
||
(In reply to Emilio Cobos Γlvarez (:emilio) from comment #42)
I'm not quite sure off-hand (I'm not familiar with this test), but I see a lot of slow style system stuff going on in places where it shouldn't, due to an old friend of ours :-)
In particular, there's a huge time spent in rebuilding the cascade data of the shadow trees when calling querySelector() from various getters from connectedCallback() and company. The issue is our old friend
Element::GetBindingURL
. What kind of elements are we poking at that are causing us to resolve style?Looking a bit closer we're querySelector'ing
.panel-arrowcontent
, which is a xul:box. Not sure whether it'd be worth changing the existing check inMayNeedToLoadXBLBinding
by a blacklist? How many elements remain with XBL bindings?
Oh I was wondering why it was so slow to querySelector for an element in the attr inheritance (and why it was even slow to do shadowRoot.firstElementChild etc). That could make sense! I will have to do a new profile push with the improvements I have locally to confirm that we are still actually making a JS reference to an element before the measurement is taken.
If so:there are only <panel>, <arrowscrollbox>, and <textbox> left - I think we could switch MayNeedToLoadXBLBinding to a whitelist instead.
That'd be probably a cheap win, though not 100% sure it explains the regression... It's most of the time in the profile looks like, so it might.
Mike, would you be able to point to exactly which point in these profiles the measurement is started / stopped (the time range we care about for the purposes of twinopen)?
Reporter | ||
Comment 44•5 years ago
|
||
Here's a profile where we don't call initializeAttributeInheritance (and thus don't require querySelector in panel https://perfht.ml/2puCZvC). This drops time in panel.js down to 3ms (filtered at https://perfht.ml/2nW2Ls4), but we are still seeing the 30% (~40ms) regression even with this change :(.
Reporter | ||
Comment 45•5 years ago
|
||
The plus side on the previous profile is I'm seeing 8ms in querySelector->GetBindingURL so I expect Bug 1585819 will be a nice improvement.
Updated•5 years ago
|
Comment 46•5 years ago
|
||
Okay, digging into the profiles. Here are the links again to the pushes (copied from comment 41):
Bad (regression with patch applied): https://treeherder.mozilla.org/#/jobs?repo=try&revision=aae1688be265a873d2fc1cc574c58fe323f4acdf&selectedJob=269471800
Before even looking at the profiles, I'm going to take a look at the scores that were generated by the two profiled runs. Unfortunately, Treeherder doesn't make these obvious, so I have to pull them out of the raw logs.
Good: twinopen.html;161.22998046875;194.497314453125
Bad: twinopen.html;218.088134765625;161.276123046875
So the second cycle for the "bad" case actually ran quite well - it does just as well as the first cycle of the "good" case. The second cycle of the "good" case also isn't that great - it's only ~24ms off from the worse cycle of the "bad" case.
So I'm going to compare the first cycles of the good and bad runs.
Good: https://perfht.ml/2Izyuqr
Bad: https://perfht.ml/355MAJo
Looking at the twinopen code, before it opens any windows, it forces a GC/CC, and then waits a full second before opening the window. This means the vast majority of these profiles is useless - we want the chunks just after the big pause. So zooming in:
Good: https://perfht.ml/330AVdi
Bad: https://perfht.ml/339QVtF
Looking at what twinopen measures, it looks like it cares about the delta between the time just before requesting that the new browser window open, and the time of the timestamp of the first MozAfterPaint fired in the newly opened window.
The thing that jumps out to me is that in the "bad" profile, the compositor looks to be waiting longer before it does a composite (which causes the MozAfterPaint to fire).
There's a big CONTENT_FRAME_TIME marker in there in the "bad" case, reporting a time of 51ms. There are a few CONTENT_FULL_PAINT_TIME markers in there as well.
In the "good" case, there aren't as many CONTENT_FULL_PAINT_TIME markers - I actually think there's only 1 each. Why?
Looking at the rasterize markers in the parent process, I also see an early rasterization in the "good" case, due to a refresh driver tick.
So I wonder if that's what's happened here - in the good case, we somehow hit a path where we were able to paint and upload layers to the compositor earlier... and in the bad case, we've lost that ability, or it's being postponed by other work.
So perhaps this is a performance cliff - the extra time required to run the connectedCallback for the panel ends up causing us to miss an opportunity to paint earlier. That's my current thinking on this, anyhow.
Reporter | ||
Comment 47•5 years ago
|
||
Thanks for the detailed analysis Mike. It sounds like it might be something we would want to consider just living with, but we could also look into:
-
lazifying some of our panel instances to limit the amount of work we need to do here. In general we should be able to wrap any [hidden] panel into an <html:template> and then change callers to check for existence before operating on the panel. This would definitely be easier on some panels than others, but here's a collection of a few that look relatively easy:
https://searchfox.org/mozilla-central/search?q=UITourHighlight&path=
https://searchfox.org/mozilla-central/search?q=ctrlTab-panel&path=
https://searchfox.org/mozilla-central/search?q=confirmation-hint&path=
https://searchfox.org/mozilla-central/search?q=panic-button-success-notification&path= -
Take the [hidden] attribute as a hint to not attach the shadow DOM, then add "hidden" to
observedAttributes
and construct things then instead of connectedCallback. This isn't as robust as XBL construction (we wouldn't detect CSS-only hidden elements), but we do seem to set this consistently on panels in browser.xhtml and then un-set it before showing the popup.
Alex, maybe start by trying (2) (combined with the "no inherited attributes" change to make attr observing simpler) and see if we can get that done without updating consumers?
Assignee | ||
Comment 48•5 years ago
|
||
Trying suggestion from comment #47, got running into assertion in debug builds (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269801884&repo=try&lineNumber=2494):
Assertion failure: sIndirectLayerTrees[child].mParent->mOptions == mOptions, at /builds/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeParent.cpp:1720
mozilla::layers::PCompositorBridgeParent::OnMessageReceived(IPC::Message const&)
Brian captured a last JS call before hitting the assertion:
0:07.51 GECKO(53166) _t/elements/panel.js 72 initialize
0:07.51 GECKO(53166) _t/elements/panel.js 57 connectedCallback
0:07.52 GECKO(53166) _ExtensionPopups.jsm 453 PanelPopup
0:07.52 GECKO(53166) _t/ext-pageAction.js 308 handleClick
0:07.52 GECKO(53166) _t/ext-pageAction.js 108 onCommand
0:07.52 GECKO(53166) _les/PageActions.jsm 937 onCommand
0:07.52 GECKO(53166) _wser-pageActions.js 726 doCommandForAction
0:07.52 GECKO(53166) _wser-pageActions.js 506 commandHandler
0:07.52 GECKO(53166) _eTest/EventUtils.js 472 synthesizeMouseAtPoint
0:07.52 GECKO(53166) _eTest/EventUtils.js 406 synthesizeMouse
0:07.52 GECKO(53166) _eTest/EventUtils.js 509 synthesizeMouseAtCenter
0:07.52 GECKO(53166) _est/browser/head.js 665 clickPageAction
So the assertion is somehow related to shadow DOM creation. Also if layout is forced by this.getBoundingClientRect() in initialize() then the problem goes away.
Emilio, it seems we do nothing illegal in the patch, so we shouldn't assert here. Could you look into the issue?
Reporter | ||
Comment 49•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #48)
Trying suggestion from comment #47, got running into assertion in debug builds (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269801884&repo=try&lineNumber=2494):
Assertion failure: sIndirectLayerTrees[child].mParent->mOptions == mOptions, at /builds/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeParent.cpp:1720
mozilla::layers::PCompositorBridgeParent::OnMessageReceived(IPC::Message const&)Brian captured a last JS call before hitting the assertion:
0:07.51 GECKO(53166) _t/elements/panel.js 72 initialize
0:07.51 GECKO(53166) _t/elements/panel.js 57 connectedCallback
0:07.52 GECKO(53166) _ExtensionPopups.jsm 453 PanelPopup
0:07.52 GECKO(53166) _t/ext-pageAction.js 308 handleClick
0:07.52 GECKO(53166) _t/ext-pageAction.js 108 onCommand
0:07.52 GECKO(53166) _les/PageActions.jsm 937 onCommand
0:07.52 GECKO(53166) _wser-pageActions.js 726 doCommandForAction
0:07.52 GECKO(53166) _wser-pageActions.js 506 commandHandler
0:07.52 GECKO(53166) _eTest/EventUtils.js 472 synthesizeMouseAtPoint
0:07.52 GECKO(53166) _eTest/EventUtils.js 406 synthesizeMouse
0:07.52 GECKO(53166) _eTest/EventUtils.js 509 synthesizeMouseAtCenter
0:07.52 GECKO(53166) _est/browser/head.js 665 clickPageActionSo the assertion is somehow related to shadow DOM creation. Also if layout is forced by this.getBoundingClientRect() in initialize() then the problem goes away.
Emilio, it seems we do nothing illegal in the patch, so we shouldn't assert here. Could you look into the issue?
More information: I can reproduce the assertion locally with https://hg.mozilla.org/try/rev/3944950226507b2f53c1ab17b16b7b5b62c49ad1 applied and ./mach mochitest browser/components/extensions/test/browser/browser_ext_pageAction_contextMenu.js
. If layout is forced after attachShadow (at https://hg.mozilla.org/try/rev/3944950226507b2f53c1ab17b16b7b5b62c49ad1#l16.102) then the assertion goes away.
Reporter | ||
Comment 50•5 years ago
|
||
OK here's a new cleaned up version with the forced layout after attachShadow (https://hg.mozilla.org/try/rev/13b8af5f81c7e29872b5538fa2847c3c101d0dc0#l16.130).
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c287af0b8bd6e312b3c4a3e158159fb067647285
Talos compare (ongoing, but good so far): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c562a8f24979ce702767164da131865c7ac564e5&newProject=try&newRevision=35676d0f656b0ab920ef3a7be1666c73031ee33e&framework=1&showOnlyImportant=1
I don't think it'd be too terrible to land with that forced layout as a temporary solution if talos is happy (with a follow up to remove that and fix the assertion), since that code doesn't run in the browser UI until a panel gets opened.
Assignee | ||
Comment 51•5 years ago
|
||
in addition to comment #49: another failure is unexpected reflow that triggers when we try to set up DOM (and force layout) on hidden attribute change (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269877721&repo=try&lineNumber=1901-1911):
19-10-05T00:58:17.055Z] 00:58:17 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu.js | unexpected reflow at initialize@chrome://global/content/elements/panel.js hit 1 times
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - Stack:
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - initialize@chrome://global/content/elements/panel.js:60:7
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - set hidden@chrome://global/content/elements/panel.js:69:14
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - ensureReady@chrome://browser/content/customizableui/panelUI.js:360:5
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - async*show/<@chrome://browser/content/customizableui/panelUI.js:228:18
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - show@chrome://browser/content/customizableui/panelUI.js:246:7
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - openMainMenu/<@resource://testing-common/CustomizableUITestUtils.jsm:95:26
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - openPanelMultiView@resource://testing-common/CustomizableUITestUtils.jsm:72:11
[task 2019-10-05T00:58:17.055Z] 00:58:17 INFO - openMainMenu@resource://testing-common/CustomizableUITestUtils.jsm:92:16
[task 2019-10-05T00:58:17.056Z] 00:58:17 INFO - @chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu.js:101:46
so apparently force layout is a perfromance issue that should be avoided.
Reporter | ||
Comment 53•5 years ago
|
||
I found what looks like a cleaner workaround for this issue than forcing layout - if we attach the shadowRoot in the constructor and then conditionally attach the arrow panel fragment and only an html:slot for non arrow panel when connected/unhidden, the assertions seem to go away and talos looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=305930af234af6544f838d47b196787b275f47f6. And specifically: https://hg.mozilla.org/try/rev/cad00209e8c83a8b6fcedc832a41a0d5d33477b9#l17.50. Obviously we need to still get to the bottom of Comment 48 but we could consider moving that into a follow up.
Assignee | ||
Comment 54•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #53)
I found what looks like a cleaner workaround for this issue than forcing layout - if we attach the shadowRoot in the constructor and then conditionally attach the arrow panel fragment and only an html:slot for non arrow panel when connected/unhidden, the assertions seem to go away
you still have force layout thing (https://hg.mozilla.org/try/rev/cad00209e8c83a8b6fcedc832a41a0d5d33477b9#l17.135), but anyways seems like a good unblocker find, will update the patch to
Comment 55•5 years ago
|
||
Haven't dug yet but, for the record:
(rr) p sIndirectLayerTrees[child].mParent->mOptions
$2 = {mUseAPZ = true, mUseWebRender = false, mUseAdvancedLayers = false, mInitiallyPaused = false}
(rr) p mOptions
$3 = {mUseAPZ = false, mUseWebRender = false, mUseAdvancedLayers = false, mInitiallyPaused = false}
Comment 56•5 years ago
|
||
The compositor with mOptions
is the one created for:
XUL* browser@0x7f6696c730e0 type="content" disableglobalhistory="true" transparent="true" class="webextension-popup-browser webextension-preload-browser" webextension-view-type="popup" tooltip="aHTMLTooltip" contextmenu="contentAreaContextMenu" autocompletepopup="PopupAutoComplete" selectmenulist="ContentSelectDropdown" selectmenuconstrained="false" remote="true" flex="1" state=[20800000] flags=[01040002] primaryframe=0x7f6696b8b150 refcount=10<>
Comment 57•5 years ago
|
||
The parent compositor that has APZ is the top level window.
Other remote loaders sent for adoption, for my own reference:
XUL* browser@0x7f669c5c17c0 contextmenu="contentAreaContextMenu" datetimepicker="DateTimePickerPanel" message="true" messagemanagergroup="browsers" selectmenulist="ContentSelectDropdown" tooltip="aHTMLTooltip" type="content" remote="true" autocompletepopup="PopupAutoComplete" autoscrollpopup="autoscroller" state=[20800000] flags=[01000002] primaryframe=0x7f66976f4988 refcount=10<>
XUL* browser@0x7f668b5bb920 contextmenu="contentAreaContextMenu" datetimepicker="DateTimePickerPanel" message="true" messagemanagergroup="browsers" selectmenulist="ContentSelectDropdown" tooltip="aHTMLTooltip" type="content" remote="true" autoscrollpopup="autoscroller" preloadedState="preloaded" nodefaultsrc="true" state=[20800000] flags=[01000002] primaryframe=0x7f6696454dd0 refcount=10<>
Reporter | ||
Updated•5 years ago
|
Comment 58•5 years ago
|
||
FWIW I think that the reparenting that goes on ends up with different compositor options is a bug, but this is the stack that explains the frame reconstruction which ends up triggering the reparenting after all.
Reporter | ||
Comment 60•5 years ago
|
||
(In reply to Emilio Cobos Γlvarez (:emilio) from comment #59)
And the reason we need to reconstruct is (you guessed it!) bug 1584935.
How fun :). Can we go ahead and make that return false in this patch, since <panel> is the last XBL binding we actually ship in the UI?
Comment 61•5 years ago
|
||
Haven't dug, but last time I checked there were some other tests / APIs that relied on it (wrongly). In particular, swapFrameLoaders didn't seem to flush frames, even though it should and relies on us eagerly creating them.
Reporter | ||
Comment 62•5 years ago
|
||
(In reply to Emilio Cobos Γlvarez (:emilio) from comment #61)
Haven't dug, but last time I checked there were some other tests / APIs that relied on it (wrongly). In particular, swapFrameLoaders didn't seem to flush frames, even though it should and relies on us eagerly creating them.
OK, probably best to hold off until XBL is fully gone before taking that then. I think we have a frontend workaround that would be OK in Comment 53 if that's easier.
Comment 63•5 years ago
|
||
In that case, the flat tree cannot possibly be changing, so we don't really need
to invalidate anything. This, in theory, is just a really minor optimization.
In practice however, the browser chrome needs it, at least for now, because XUL
elements get frames really early (because we don't have lazy frame construction
for XUL, bug 1584935), and because destroying some kinds of frames (like panels)
does have side effects (they're popups), even though ideally they shouldn't.
Comment 64•5 years ago
|
||
So the patch I posted fixes the assertion and the silly reframing going on. However there are a few extra non-fatal assertions coming from here -> here -> here because the web extensions code expect an uninitialized panel, but it is initialized from the connected callback.
Not 100% sure what's up with that, but I suspect that one you can try to figure out? If you cannot I'm happy to dig a bit more.
Reporter | ||
Comment 65•5 years ago
|
||
(In reply to Emilio Cobos Γlvarez (:emilio) from comment #64)
So the patch I posted fixes the assertion and the silly reframing going on. However there are a few extra non-fatal assertions coming from here -> here -> here because the web extensions code expect an uninitialized panel, but it is initialized from the connected callback.
Not 100% sure what's up with that, but I suspect that one you can try to figure out? If you cannot I'm happy to dig a bit more.
Do you see the "Creating widget for MenuPopupFrame with children" assertion from ./mach mochitest browser/components/extensions/test/browser/browser_ext_pageAction_contextMenu.js
? I don't see it on my latest version of the patch, but it's quite a bit different from the one we sent originally so maybe something unrelated has fixed it.
Reporter | ||
Updated•5 years ago
|
Comment 66•5 years ago
|
||
Yes, that's the assertion. Maybe, that'd be great if it's fixed.
Reporter | ||
Comment 67•5 years ago
|
||
Magnus, just a heads up that there are a number of CSS changes that will be needed for <panel> as a Custom Element (using Shadow Parts instead of XBL traversal in CSS). See examples at: https://phabricator.services.mozilla.com/D35040.
Comment 69•5 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c3489dfee60 Replace panel and arrowpanel bindings with a custom element r=bgrins
Reporter | ||
Updated•5 years ago
|
Comment 70•5 years ago
|
||
bugherder |
Comment 71•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/998a92b4905b Don't invalidate style / layout of the host if it has no children. r=smaug
Assignee | ||
Updated•5 years ago
|
Comment 72•5 years ago
|
||
bugherder |
Description
•