Convert menuitem bindings to Custom Element
Categories
(Toolkit :: UI Widgets, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
Attachments
(4 files)
https://bgrins.github.io/xbl-analysis/tree/#menuitem In general I think we can wait to render the menuitems until a popupshowing event happens, which will let us bypass potential perf issues due to a ton of hidden menuitems getting rendered at startup. The menuitem-base binding would need to stick around since menu-base extends it (although those two could be folded together, I think).
Assignee | ||
Comment 1•6 years ago
|
||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
> In general I think we can wait to render the menuitems until a popupshowing
> event happens, which will let us bypass potential perf issues due to a ton
> of hidden menuitems getting rendered at startup.
Note that you can't do this with popups that have the sizetopopup attribute (such as all menulists) as the size of the button portion is dependent on the size of the items.
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Neil Deakin from comment #2) > > In general I think we can wait to render the menuitems until a popupshowing > > event happens, which will let us bypass potential perf issues due to a ton > > of hidden menuitems getting rendered at startup. > > Note that you can't do this with popups that have the sizetopopup attribute > (such as all menulists) as the size of the button portion is dependent on > the size of the items. Ugh, so this is another case where we expect to run JS as a result of layout happening (on the parent menulist or whatever menu node has [sizetopopup] set). Maybe we can get to perf parity here by telling the menuitem to render as a results of XBL construction on the parent, but this is going to come up again then when we try to migrate the parent. I wonder if we could wire up a method with xpcom to call into JS in nsMenuFrame::SizeToPopup or similar so the menulist could explicitly render children if needed.
Comment 4•6 years ago
|
||
Can we remove platform support for the "sizetopopup" attribute?
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to :Paolo Amadini from comment #4) > Can we remove platform support for the "sizetopopup" attribute? I don't know about for chrome, but I assume we need to keep it in-content <select> menulists since this is seen on web pages. For example: `data:text/html,<select><option>h</option><option>longstring longstring longstring</option></select>` renders as wider than `data:text/html,<select><option>h</option></select>`. But that could easily be detected and eagerly rendered in connectedCallback for that case (we have a [popuponly] attribute for this dropdown). There's also work ongoing to change how the in-content select dropdowns work so that it wouldn't rely on these elements / bindings in Bug 1421229.
Comment 6•6 years ago
|
||
Hm, probably worth looking into whether HTML <select> in the content process and XUL <menulist> actually use the same code path for deciding their own size, and if so, whether this can be changed so that they diverge, if this makes the XUL layout issue above easier to solve.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
From https://bugzilla.mozilla.org/show_bug.cgi?id=1519495#c2:
When moving anonymous content to document DOM, add
role="none"
sinceXULMenuitemAccesible
should ignore them.
Assignee | ||
Comment 9•5 years ago
|
||
From https://bugzilla.mozilla.org/show_bug.cgi?id=1519495#c0:
The following bindings can be converted into multiple custom elements, tie to one
menuitem
tag name and differentis
values.
- menuitem
- menuitem-iconic
- menuitem-iconic-noaccel
They have
<children>
but we won't be moving them. Rather, we can simply prepend/append nodes before and after the children. They might not be used and if so they can be cleaned up.raw meeting note:
menuitem
It’s a base binding. Uses <content> but not children. Two extended bindings
menuitem-iconic
We need to figure out if <children> is actually used here (inmenuitem[type="checkbox"], menuitem[type="radio"] (content/xul.css), menuitem.menuitem-iconic (content/xul.css), menuitem:not([type]) (viewsource/viewsource.css)
)
Could confirm with a try push, but this was originally added for a single case: https://hg.mozilla.org/mozilla-central/rev/c868ce24344c#l1.23
<children /> is a direct child of <content>; we don’t need to move it and hit layout assertion.
menuitem-iconic-noaccel
This is overloaded with iconic and plain menuitem, but if we don’t need slotting above then we could make this a runtime check and toggle the correct <content> based on that. May be perf issues to deal with since there’s a ton of these not visible initially
Also:
menuitem-base
[implements], 2 children, totally doable but can’t be removed yet for same reason as basecontrol. Bug can be filed to track dependency.
Assignee | ||
Comment 10•5 years ago
|
||
Neil and Gijs, to simplify things I'm considering whether we can remove the <children /> slotting for menuitem-iconic
at https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/toolkit/content/widgets/menu.xml#180. I did a try push where I throw if there is a child node at XBL construction time and so far I'm just seeing a single test which I think could be changed to use a <box> or some other element (layout/xul/test/test_bug372685.xul): https://treeherder.mozilla.org/#/jobs?repo=try&revision=695bcb3417a1350e1ee5dcce81e6a2b14a109668&selectedJob=227738607.
Is there any case you can think of where we want to allow the page to put arbitrary children inside of a menuitem and have it slotted before the menu-accel-container? Every <menuitem /> I've seen at least in markup is self closing with no children.
Comment 11•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #10)
Is there any case you can think of where we want to allow the page to put arbitrary children inside of a menuitem and have it slotted before the menu-accel-container?
Nope. We should change bindings / subclass if so (like we did for the icons in the context menu).
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to :Gijs (he/him) from comment #11)
(In reply to Brian Grinstead [:bgrins] from comment #10)
Is there any case you can think of where we want to allow the page to put arbitrary children inside of a menuitem and have it slotted before the menu-accel-container?
Nope. We should change bindings / subclass if so (like we did for the icons in the context menu).
Thanks, will remove it in Bug 1527115.
Assignee | ||
Comment 13•5 years ago
|
||
Happily, talos looks good with this: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5904dc025a2bc21b445e6ec2b54503e0183ae1ed&newProject=try&newRevision=bea727a2e7deb379f5a9d8c5a86e11b2cd0c61d1&framework=1. I see < 1ms of time in the class spent at startup (since we can lazily create DOM in most cases sans Comment 2). Still a couple of mochitest failures to work through but this is looking pretty close.
Assignee | ||
Comment 14•5 years ago
|
||
Emilio, there's something weird I'm seeing here where we aren't getting a Custom Element attached in certain circumstances:
STR:
- Apply https://phabricator.services.mozilla.com/D9322
- Open about:preferences
- Scroll down to "Fonts and Colors" and click the "Size" dropdown
- See "TypeError: el.render is not a function"
A few notes:
- The Custom Element seems to just not get attached -
instanceof customElements.get("menuitem")
is false, no properties available, etc. - This gets fixed by switching CDATA to an <html:template> (Bug 1545962)
- This call is happening in popupshowing at https://hg.mozilla.org/try/rev/e0fdf0ed517a0371e840f9728073f4100bca2c22#l6.23. The <menuitem /> elements themselves seem to have no Custom Element attached (the same call to
el.render()
works fine with all the other menuitems that I've tested). - Looks like TB is seeing some similar behavior with <menu> in https://bugzilla.mozilla.org/show_bug.cgi?id=1545263#c8. Though I'm not sure if this is also a CDATA issue.
Assignee | ||
Comment 15•5 years ago
|
||
The way CDATA is being used here is:
- There's a box with CDATA at https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/browser/components/preferences/in-content/main.xul#19 (the broken menuitems are inside it at https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/browser/components/preferences/in-content/main.xul#146)
- When the section is shown, we take the text from CDATA and send it into parseXULToFragment then replace the box (https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/browser/components/preferences/in-content/preferences.js#50 / https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/toolkit/content/customElements.js#411)
Comment 16•5 years ago
|
||
Does manually upgrading the menuitem work out of curiosity?
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)
Does manually upgrading the menuitem work out of curiosity?
No it doesn't, see screenshot
Comment 18•5 years ago
|
||
Alright, I'll take a look, probably tomorrow.
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
I was working with or debugging these in the process of doing the previous
changeset, so took the oppurtunity to switch these to add_task and do some
miscellaneous cleanup.
Depends on D9322
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
Alright, I'll take a look, probably tomorrow.
Thank you! Also, sorry to complicate the STR but if Bug 1545962 / https://hg.mozilla.org/integration/autoland/rev/8ce092764f60 sticks you'll have to back that out as well - that's migrating CDATA to <template> in preferences which we wanted to do anyway, and also fixes this bug. I still think we need to figure out what's going on here since I'm not sure this is only triggered by CDATA - it looks like TB is hitting something similar with a different case which they suspect has to do with importing nodes from a another document (https://bugzilla.mozilla.org/show_bug.cgi?id=1545263#c10).
Assignee | ||
Comment 21•5 years ago
|
||
Interesting, if I replace this line https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/browser/components/preferences/in-content/preferences.js#50:
let frag = MozXULElement.parseXULToFragment(template.firstChild.data);
with:
let frag = document.importNode(MozXULElement.parseXULToFragment(template.firstChild.data), true);
it starts working again. The weird thing is that there are other custom elements in there that do get attached like the sibling menulist (document.getElementById("defaultFont") instanceof customElements.get("menulist")
returns true).
Assignee | ||
Comment 22•5 years ago
|
||
Here's an easier STR for the browser console that doesn't require loading about:preferences (again, this requires the patch here so that customElements.get("menuitem")
is defined):
if (document.getElementById("defaultFontSize")) {
document.getElementById("defaultFontSize").remove();
}
var frag = MozXULElement.parseXULToFragment(`
<menulist id="defaultFontSize" delayprefsave="true">
<menupopup>
<menuitem value="9" label="9"/>
</menupopup>
</menulist>
`);
document.documentElement.append(frag);
console.log(document.getElementById("defaultFontSize") instanceof customElements.get("menulist")); // true
console.log(document.getElementById("defaultFontSize menuitem") instanceof customElements.get("menuitem")) // false
Comment 23•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #22)
Here's an easier STR for the browser console that doesn't require loading about:preferences (again, this requires the patch here so that
customElements.get("menuitem")
is defined):
[...]
These STR don't reflect the issue. document.getElementById("defaultFontSize menuitem")
returns null. Changing it to document.querySelector("#defaultFontSize menuitem")
it works.
So I'll try with the about:preferences str for now :)
Comment 24•5 years ago
|
||
So I tried and managed to reproduce this, but it's very intermittent and I couldn't catch it on a debugger easily. I'll try a bit harder later.
Assignee | ||
Comment 25•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #24)
So I tried and managed to reproduce this, but it's very intermittent and I couldn't catch it on a debugger easily. I'll try a bit harder later.
FWIW I can reproduce basically every time in an opt (+ artifact) build on OSX using STR in Comment 14 + the backout in Comment 20.
Comment 26•5 years ago
|
||
Yeah, I managed to get it on a debug build and I have rr... I have an hypothesis. If in the menuitem custom element constructor or connectedCallback() you define a property (like this.foo = "bar"
), does the bug go away?
Comment 27•5 years ago
|
||
(What's happening is that the JS wrapper that gets created in the menu connected callback (when you access this.children via setSelectedIndex get gc'd... I thought it'd be trivial to reproduce in content but haven't found the right test-case yet)
Assignee | ||
Comment 28•5 years ago
|
||
Adding this.foo = "bar"
to both the connectedCallback and the constructor fixes the problem!
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #28)
Adding
this.foo = "bar"
to both the connectedCallback and the constructor fixes the problem!
To clarify, the problem is fixed when adding it to just the constructor or just the connectedCallback (not needed to add it to both at the same time).
Comment 30•5 years ago
|
||
Eventually this test-case throws a TypeError: document.body.firstElementChild.myFunction is not a function
.
If you remove the:
(function() {
let foo = document.body.firstElementChild;
}())
Or do the property dance, it starts working as expected.
The issue is that we create the wrapper with the canonical proto, and then modify its prototype. If we do it in the other order it just works, because of:
Jandem very helpfully pointed out to me that we have a bug on file for this kind of issue (bug 1354015). We do preserve wrappers when you add a property, but not when you change proto.
Well that was fun to debug.
Updated•5 years ago
|
Comment 31•5 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/37d758a90ed9 Convert <menuitem> bindings to a Custom Element r=surkov
Comment 32•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•