Closed Bug 1518932 Opened 5 years ago Closed 5 years ago

Convert menulist to custom element

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1518932 - Convert menulist to custom element
47 bytes, text/x-phabricator-request
Details | Review
Bug 1518932 - Create BaseControlMixin, allowing BaseControl to inherit other native XULElement classes
47 bytes, text/x-phabricator-request
Details | Review

This might work in the way I intend for bug 1514926 to work; the only child of <menulist> element should be <menupopup>, and we will only be inserting things before it, instead of moving the child.

If this works we can land

https://phabricator.services.mozilla.com//D14925

here since I would need it to use for the new custom element to inherit XULMenuElement.

See Also: → 1513334
This custom element replaces XBL <content> usage by directly
prepend the two needed child nodes when the element is connected.

This is doable because

a) There isn't any direct access of child nodes under <menulist>.
   Everyone seems to access via .menupopup, which is usually the only child.
   (One instance of that indentified is removed)
b) We don't need to move the children under <menulist>. If we need to and if
   the child is a <xbl:children> (which could happen if <menulist> is inside
   a XBL <content> that just get cloned to the document), the layout will
   get very confused and crash (see finding in bug 1514926)
Attachment #9035537 - Attachment is obsolete: true
Attachment #9035537 - Attachment is obsolete: false
Priority: -- → P2
Component: XBL → XUL Widgets
Priority: P2 → P5
Product: Core → Toolkit

I am looking at a very strange behavior here:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221016360&repo=try&lineNumber=19195

This test calls menulist.appendItem() to get a <menuitem> constructed, set its .image, and assert that the value of the .image gets set on the menulist.

The image property is defined in the basetext XBL binding. Here is the interesting part: how did the <menuitem> binding gets constructed, if the popup is not opened?

I put a dump(("image" in item) + "\n"); right after the appendChild() line and it seems that appending a child into existing XBL anonymous content can magically cause the binding to construct, while the CE menulist does not pose this kind of magic. I have not locate the DOM code that does this though so I might be wrong, but this is very troubling if so since I don't have any way in CE to force XBL construction on a non-visible element.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)

... and it's fixed because :bgrins quickly remind me of this:

https://searchfox.org/mozilla-central/rev/7adb490485eff0783071a3e132005bceeb337461/toolkit/content/customElements.js#118,120-124

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6562564a8015469d163a0155883a2755b57d4ef9

Failures are mostly fixed. I am not sure about I set up the nsIDOMXULMenuListElement interface right though. a11y test still fails.

There are two more things to note:

  • menulist binding seems to be able to keep the menuitem bindings attached even though they are not visible under menupopup. The menulist CE don't have that ability. I had to change instances of menulist.selectedItem.value to menulist.value for things to work. I don't really know why the previous getter work (other than the fact the selectedItem is kept alive for some unknown reasons.
  • sizetopopup is a major pain. The value set by the binding can be pick up by layout, while CE can only set it after the fact (it seems) and force layout to pick it up. It would still partially work (see the reftest change). We may want to move the attribute to the caller (there are many 😢) and/or change the default, but I don't think we should address that in this bug (unless we don't want to land the hack.)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #8)

  • sizetopopup is a major pain. The value set by the binding can be pick up by layout, while CE can only set it after the fact (it seems) and force layout to pick it up. It would still partially work (see the reftest change). We may want to move the attribute to the caller (there are many 😢) and/or change the default, but I don't think we should address that in this bug (unless we don't want to land the hack.)

I decided to remove the hack and properly fix this in nsMenuPopupFrame and nsMenuFrame. The fix only conservatively keys to frames generated by <menulist> just to keep the scope in check.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae7a527a659b31ae5622b3a7c2cffab538d89e2

sizetopopup is set to "pref" by default by the menulist XBL binding, however
when converting the binding to custom element, it did not set the attribute value
at a time that is early enough.

This patch updates nsMenuPopupFrame and nsMenuFrame so that it considers
<menulist> with unset sizetopopup attribute as equal to "pref" to avoid
the problem above.

The problem can be detected by the reftest
layout/reftests/xul/menulist-shrinkwrap-2.xul
in which the parent box won't update its width even when <menulist>
reframes with the correct sizetopopup value.

The sizetopopup attribute is never meant to be set dynamically;
the fix here does not allow us to do so.

It turned out I had to explicitly reference Ci.nsIDOMXULSelectControlElement when calling implementCustomInterface. Passing Ci.nsIDOMXULMenuListElement alone is not enough.

Attachment #9036204 - Attachment is obsolete: true

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e810c789c59eadfb18afd2f9223aee6303f689e6

This is super weird — it showing test failures that should have been fixed, and shouldn't have been passed on any platforms, but it only fails on some of the platforms. This is going to take a while 😖

Actually, this should fix a11y on Windows so let's see if it could fix something else

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9078641ff336999fda160c5e82623683788e39

Depends on: 1519038
See Also: 1519038 →

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)

Pushes for talos tests

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=43067715c57f&newProject=try&newRevision=1495d96f413c&framework=1

A little bit more context:

So there are four <menulist>s in browser.xul with three hidden. They are WebRTC device selection drop-downs in popup-notifications.inc. The patch here will make us running extra constructions for these three elements.

What's even worse is that at the time in bug 823443, PopupNotifications.jsm is implemented such that it will remove and re-append the <popupnotification>s to show. I can see the menulist XBL binding gets constructed and immediately destructed three times when the popup is first shown. Sadly that useless construction will now happen during start-up.

I would like to avoid creeping this bug too much so unless that's proven to be a performance problem, I will not try to deal with it (as a dependency or in this bug). Nonetheless I will file a bug for this.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)

Pushes for talos tests

It turned out we regressed about:preferences by a lot. That's not unexpected — should have realized that earlier. 😞
I would need to find a way to defer the CE construction in the hidden panels.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)

Pushes for talos tests

It turned out we regressed about:preferences by a lot. That's not unexpected — should have realized that earlier. 😞
I would need to find a way to defer the CE construction in the hidden panels.

Is it possible that we could optimize the connectedCallback (sharing a document fragment that gets cloned, for example) to avoid having to defer construction for hidden elements?

Also, are you seeing similar numbers locally on about:preferences (I guess 6-10ms in the CE)? I've used ./mach talos-test -a about_preferences_basic --cycles 1 --gecko-profile locally in the past for this (or the --talos-profile flag in try fuzzy syntax without rebuilds).

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)

Pushes for talos tests

It turned out we regressed about:preferences by a lot. That's not unexpected — should have realized that earlier. 😞
I would need to find a way to defer the CE construction in the hidden panels.

Is it possible that we could optimize the connectedCallback (sharing a document fragment that gets cloned, for example) to avoid having to defer construction for hidden elements?

Just done that, let's see how it goes, thanks for the suggestion.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=43067715c57f&newProject=try&newRevision=db1c9f23e499&framework=1

Also, are you seeing similar numbers locally on about:preferences (I guess 6-10ms in the CE)? I've used ./mach talos-test -a about_preferences_basic --cycles 1 --gecko-profile locally in the past for this (or the --talos-profile flag in try fuzzy syntax without rebuilds).

Unfortunately I am using debug build locally so I won't be able to do a meaningful talos measurement.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #22)

Just done that, let's see how it goes, thanks for the suggestion.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=43067715c57f&newProject=try&newRevision=db1c9f23e499&framework=1

There are improvements but that doesn't look enough though.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24)

Compare with patches applied on top of bug 1520350

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=69ecb01f1d1e&newProject=try&newRevision=514f70d51012&framework=1

This demonstrates bug 1520350 could fix the perf regression here. All of the speed differences returns with low confidence except one. I am retriggering more tests to get the data point for that one.

BTW I have removed the use of XPCOMUtils with small vanilla JS. This is the artifact build test run on top of bug 1520350.

https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream%40gmail.com&selectedJob=222567906

Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b77df8435ab1
Create BaseControlMixin, allowing BaseControl to inherit other native XULElement classes r=bgrins
https://hg.mozilla.org/integration/autoland/rev/c1032d34b5e0
Convert menulist to custom element r=paolo

https://hg.mozilla.org/mozilla-central/rev/b77df8435ab1
https://hg.mozilla.org/mozilla-central/rev/c1032d34b5e0

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Regressions: 1540123
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: