Closed Bug 1397876 Opened 7 years ago Closed 5 years ago

Replace the panel XBL bindings with Custom Elements

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: bgrins, Assigned: surkov)

References

Details

(Whiteboard: [xbl-custom-element][rebase-needed])

Attachments

(3 files, 10 obsolete files)

Bug 1397876 - Replace panel and arrowpanel bindings with a custom element
47 bytes, text/x-phabricator-request
Details | Review
Stack that causes the frame reconstruction (and thus the reparenting of the layer trees)
17.44 KB, text/plain
Details
Bug 1397876 - Don't invalidate style / layout of the host if it has no children.
47 bytes, text/x-phabricator-request
Details | Review 
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
This patch is very WIP, but it appears the hamburger panel is opening and positioning itself correctly (others are broken)
Component: General → XBL
Product: Firefox → Core
Whiteboard: [xbl-custom-element]
Depends on: 1422465
Depends on: 1446961
Whiteboard: [xbl-custom-element] → [xbl-custom-element][rebase-needed]
Depends on: 1505489
Summary: Investigate replacing the panel XBL bindings with a Custom Element → Replace the panel XBL bindings with Custom Elements

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).

See Also: → 1495621

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

This is blocked on Shadow Parts (Bug 1505489) but marking it as assigned since Tim's patch looks good besides that.

Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #8905648 - Attachment is obsolete: true
Attachment #9060132 - Attachment description: Bug 1397876 - Refactor browser panel CSS to not traverse panel shadow DOM. → Bug 1397876 - Refactor browser panel CSS to reduce shadow DOM traversal
Type: enhancement → task
Attachment #9060132 - Attachment description: Bug 1397876 - Refactor browser panel CSS to reduce shadow DOM traversal → Bug 1397876 - Refactor browser panel CSS to reduce shadow DOM traversal.

Taking this out of my queue until I have more time later on to take a look at it.

Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW

(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?

(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?

(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?

(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.

(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

Apparently neither UploadDiff nor specified Differential Revision in the patch works. Are you sure you want the changes as a separate patch?

(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).

Attachment #9072189 - Attachment is obsolete: true
Attachment #9072197 - Attachment is obsolete: true
Attachment #9054093 - Attachment is obsolete: true
Attachment #9060132 - Attachment is obsolete: true
Attachment #9071656 - Attachment is obsolete: true

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.

(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.

(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.

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.

Flags: needinfo?(emilio)

Does https://phabricator.services.mozilla.com/D35585 help by any chance? Seems like a pretty similar situation.

Flags: needinfo?(emilio) → needinfo?(surkov.alexander)
Depends on: 1569375
Attachment #9072200 - Attachment is obsolete: true
Attached patch skip-attr-inherit.patch (obsolete) — — Splinter Review

Tim, this could apply onto the second commit in the stack to lower the amount of attribute inheritance.

(In reply to Brian Grinstead [:bgrins] from comment #32)

Created attachment 9091201 [details] [diff] [review]
skip-attr-inherit.patch

Tim, 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.

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.

Attachment #9072201 - Attachment is obsolete: true
Attachment #9091201 - Attachment is obsolete: true

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?

Flags: needinfo?(surkov.alexander) → needinfo?(emilio)

(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).

(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 the overflow: 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.

(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.

Flags: needinfo?(emilio)

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):

Same base but with the <panel> patch applied:

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.

Flags: needinfo?(mconley)
Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio)

(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 in MayNeedToLoadXBLBinding 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)?

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 :(.

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.

See Also: → 1585819
Attachment #9097424 - Attachment is obsolete: true

Okay, digging into the profiles. Here are the links again to the pushes (copied from comment 41):

Good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64dd2a14912007055e7915e5f0fbada05769600e&selectedJob=269471481

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.

Flags: needinfo?(mconley)

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:

  1. 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=

  2. 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?

Flags: needinfo?(surkov.alexander)

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?

Flags: needinfo?(surkov.alexander) → needinfo?(emilio)

(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 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?

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.

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.

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.

Yeah, hopefully the assertion in Comment 48 isn't too bad to fix

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.

(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

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}

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<>

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<>

See Also: → 1586872

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.

And the reason we need to reconstruct is (you guessed it!) bug 1584935.

(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?

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.

(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.

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.

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.

Flags: needinfo?(emilio) → needinfo?(surkov.alexander)

(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.

Flags: needinfo?(emilio)

Yes, that's the assertion. Maybe, that'd be great if it's fixed.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(mkmelin+mozilla)

Thx Brian! Filed bug 1586936.

Flags: needinfo?(mkmelin+mozilla)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c3489dfee60
Replace panel and arrowpanel bindings with a custom element r=bgrins
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Component: XBL → XUL Widgets
Product: Core → Toolkit

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
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
Flags: needinfo?(surkov.alexander)
Regressions: 1591947
Regressions: 1633430
Regressions: 1642376
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: