1500626 - Convert menuitem bindings to Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1500626 - Convert <menuitem> bindings to a Custom Element

Comment 2

[Neil Deakin]

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

Comment 3

[Brian Grinstead [:bgrins]]

(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

[:Paolo Amadini]

Can we remove platform support for the "sizetopopup" attribute?

Comment 5

[Brian Grinstead [:bgrins]]

(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

[:Paolo Amadini]

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.

Comment 8

[Brian Grinstead [:bgrins]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1519495#c2:

https://searchfox.org/mozilla-central/rev/c43240cef5829b8a2dec118faff8a5e1fec6ae1b/accessible/xul/XULMenuAccessible.cpp#41

When moving anonymous content to document DOM, add role="none" since XULMenuitemAccesible should ignore them.

Comment 9

[Brian Grinstead [:bgrins]]

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 different is 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 (in menuitem[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.

Comment 10

[Brian Grinstead [:bgrins]]

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

[:Gijs (away/busy until Mar 10)]

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Brian Grinstead [:bgrins]]

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.

Comment 14

[Brian Grinstead [:bgrins]]

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.

Comment 15

[Brian Grinstead [:bgrins]]

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

[Emilio Cobos Álvarez [:emilio]]

Does manually upgrading the menuitem work out of curiosity?

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: menuitem-ce-console.png

(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

[Emilio Cobos Álvarez [:emilio]]

Alright, I'll take a look, probably tomorrow.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: Bug 1500626 - Refactor a couple of menu mochitests

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

Comment 20

[Brian Grinstead [:bgrins]]

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

Comment 21

[Brian Grinstead [:bgrins]]

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

Comment 22

[Brian Grinstead [:bgrins]]

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

[Emilio Cobos Álvarez [:emilio]]

(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

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 25

[Brian Grinstead [:bgrins]]

(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

[Emilio Cobos Álvarez [:emilio]]

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

[Emilio Cobos Álvarez [:emilio]]

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

Comment 28

[Brian Grinstead [:bgrins]]

Adding this.foo = "bar" to both the connectedCallback and the constructor fixes the problem!

Comment 29

[Brian Grinstead [:bgrins]]

(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

[Emilio Cobos Álvarez [:emilio]]

Attachment: testcase that shows the issue

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:

https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/bindings/Codegen.py#3895

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.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37d758a90ed9
Convert <menuitem> bindings to a Custom Element r=surkov

Comment 32

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/37d758a90ed9