Convert menuitem bindings to Custom Element

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
5 days ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks 1 bug, Regressed 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

7 months ago
Priority: -- → P3

Comment 2

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

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

7 months ago
Can we remove platform support for the "sizetopopup" attribute?
(Assignee)

Comment 5

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

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

7 months ago
Depends on: 1502947
(Assignee)

Updated

4 months ago
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1519495
(Assignee)

Comment 8

3 months ago

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.

(Assignee)

Comment 9

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

(Assignee)

Updated

3 months ago
Depends on: 1519498
(Assignee)

Updated

3 months ago
Depends on: 1526824
(Assignee)

Updated

3 months ago
Depends on: 1526826
(Assignee)

Updated

3 months ago
Depends on: 1465457
(Assignee)

Comment 10

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

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)

Comment 11

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

Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

3 months ago
Depends on: 1527115
(Assignee)

Comment 12

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

Flags: needinfo?(enndeakin)
(Assignee)

Updated

3 months ago
Depends on: 1527105
(Assignee)

Updated

3 months ago
Depends on: 1527680
(Assignee)

Updated

29 days ago
Depends on: 1545962
(Assignee)

Comment 13

28 days 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: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 14

27 days ago

Emilio, there's something weird I'm seeing here where we aren't getting a Custom Element attached in certain circumstances:

STR:

A few notes:

Flags: needinfo?(emilio)

Does manually upgrading the menuitem work out of curiosity?

(Assignee)

Comment 17

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

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

Attachment #9018764 - Attachment description: Bug 1500626 - WIP - convert <menuitem> to Custom Element → Bug 1500626 - Convert <menuitem> bindings to a Custom Element
(Assignee)

Comment 19

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

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

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

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

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

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

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

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?

Flags: needinfo?(bgrinstead)

(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

26 days ago

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

Flags: needinfo?(bgrinstead)
(Assignee)

Comment 29

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

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.

Flags: needinfo?(emilio)
Attachment #9060184 - Attachment mime type: text/plain → text/html

Comment 31

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

17 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 17 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1549931

Updated

10 days ago
Regressions: 1549634

Updated

6 days ago
Regressions: 1551412

Updated

5 days ago
Regressions: 1551781
You need to log in before you can comment on or make changes to this bug.