1531870 - [de-xbl] export menupopup as a base custom element class

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Paul Morris [:pmorris]]

This is the 'popup' binding which corresponds to '<menupopup>'. ('<popup>' was deprecated, so this might be a good opportunity to bring the names back in line.)

See this comment for the plan here. In short: convert the popup binding to MozElements.Popup or similar, but don't do customElements.define("menupoup") yet. That way Thunderbird and Firefox de-xbl work can both go forward without stepping on toes.

Comment 1

[Brian Grinstead [:bgrins]]

One thing to figure out here is what to do with the anonymous content. <popup> uses slotting (https://github.com/bgrins/xbl-analysis/blob/255115615ec59c7d3eaf8395cb493594fea59aec/elements/generated/Popup.js#L39-L43) so we either need to convert this to Shadow DOM or switch to normal DOM at connectedCallback (which requires moving contents into the .popup-internal-box element).

The former may have some issues with slotted contents that use XBL, and the latter might require rewriting callers from JS who think the element they added in the markup is the direct child of the menupopup.

Comment 2

[Brian Grinstead [:bgrins]]

The other issue with using Shadow DOM is that until we have Shadow Parts (Bug 1505489), then document sheets won't be able to style the anonymous .popup-internal-box element: https://searchfox.org/mozilla-central/search?q=popup-internal-box&path=

Comment 3

[Brian Grinstead [:bgrins]]

Given that there'd be less unknowns, I'd probably give a try to some "manual slotting" like:

this.appendChild(MozXULElement.parseXULToFragment(`<arrowscrollbox class="popup-internal-box" flex="1" orient="vertical" smoothscroll="false" />`));
let arrowscrollbox = this.lastElementChild;
// Append every child into arrowscrollbox.

As I mentioned in Comment 1, the problem here is that there's probably callers that dynamically add menuitems into the menupopup after it's added, or look up childNodes expected them to be menuitems. The former could be fixed by either adding an appendMenuItem function and adapting callers to use that, or by setting up a MutationObserver to detect added menu items and then move them.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Paul Morris [:pmorris] from comment #0)

This is the 'popup' binding which corresponds to '<menupopup>'. ('<popup>' was deprecated, so this might be a good opportunity to bring the names back in line.)

Good idea, let's go ahead and officially remove the popup node here. I think that means:

• Replace this comment with <menupopup> https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/layout/xul/nsMenuPopupFrame.cpp#688
• Remove these lines: https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/toolkit/content/xul.css#300-302
• Make the Custom Element have MozMenuPopup instead of MozPopup as the name

Comment 5

[Paul Morris [:pmorris]]

Attachment: wip-untested-de-xbl-popup-0.patch

Thanks for the guidance. Here's a start, still an untested WIP. And still need to handle callers dynamically adding menuitems into the menupopup and looking up childNodes expecting them to be menuitems. (I'm out of time for the week.)

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 9047868 [details] [diff] [review]
wip-untested-de-xbl-popup-0.patch

Review of attachment 9047868 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks like a nice first pass. Left a couple of comments below.

::: toolkit/content/widgets/menupopup.js
@@ +34,5 @@
> +        connectedCallback() {
> +            if (this.delayConnectedCallback()) {
> +                return;
> +            }
> +            this.textContent = "";

We won't want to clear it out in this case since we actually want to loop over the children to move them into scrollbox.

@@ +39,2 @@
>  
> +            this.scrollBox = MozXULElement.parseXULToFragment(`

here the scrollBox is a document fragment. You probably first want to append it into the DOM and then get it as a reference as a dom node. You could also just create a DOM node directly, but do see the caveat at https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/toolkit/content/customElements.js#244-269 as for why we typically use parseXULToFragment. It's possible we are OK with the delayed XBL construction in this case.

Comment 7

[Paul Morris [:pmorris]]

Attachment: wip-untested-de-xbl-popup-1.patch

Umm... forget that last patch. The popup binding is not the only binding in popup.xml.

Comment 8

[Paul Morris [:pmorris]]

Attachment: wip-untested-de-xbl-popup-2.patch

Thanks for the quick feedback. I've made those changes.

I looked into the MutationObserver approach (to handling callers working with menuitems), but we'd still need to deal with callers iterating through "menupopup.children", etc. And there are a lot of callers so it would be great to avoid having to change what they do.

So I think it would be cleaner to just override the DOM methods involving child nodes, to delegate those calls to the child element that will contain the menuitem elements. That's what I've done in this WIP patch.

(There may be more methods to include to be comprehensive. I did the ones I saw in a search for "menupopup".)

Patch still needs testing, but I wanted to post it for feedback on the approach. (Still need to figure out where to include the 'menupopup.js' script.)

If this seems like a good general solution (temporary, while waiting for Shadow Parts) then we could create a mixin function to add these methods to any custom elements that need them. (Passing a selector for the "slot" child element to the mixin function.)

(Hmm, I see :bgrins is away until March 11.)

Comment 9

[Paul Morris [:pmorris]]

Attachment: de-xbl-popup-3.patch

Here's another iteration that moves the delegation of method calls and property accesses to a function. That function could be moved somewhere that's more generally accessible if we want to use this approach for faking shadow parts in other places. (Until shadow parts arrives and we can use that instead.)

I tested the basic functionality with one of my Thunderbird/Calendar patches that depends on this, and it worked as expected. But that doesn't add or remove children dynamically. So more testing is needed if this is the approach we want to take.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 9050613 [details] [diff] [review]
de-xbl-popup-3.patch

Review of attachment 9050613 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/menupopup.js
@@ +6,5 @@
>  
> +// This is loaded into all XUL windows. Wrap in a block to prevent
> +// leaking to window scope.
> +{
> +    /**

nit: 2 spaces offset

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 9050613 [details] [diff] [review]
de-xbl-popup-3.patch

Review of attachment 9050613 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding the request to Alex - he was going to play around with this and see if it would work for our extended popup bindings (places-popup-base and places-popup-arrow)

Comment 12

[alexander :surkov (:asurkov)]

I find the idea interesting, however it makes tree unsymmetrical, if I read code right, since it doesn't override parent/parentNode on reinserted child elements, which feels errorprone. Does CE slotting is badly broken so that we want to avoid it here?

Comment 13

[Brian Grinstead [:bgrins]]

There are two main problems with SD slotting:

1. We can't directly style anonymous content inside of it from document style sheets (for example, in this case stuff that digs into popup-internal-box https://searchfox.org/mozilla-central/search?q=popup-internal-box&path=css)
2. It breaks if XBL content gets slotted in (https://bugzilla.mozilla.org/show_bug.cgi?id=1515518 / https://groups.google.com/forum/#!msg/firefox-dev/DLMgaSX8_Hk/b43_aC6EBQAJ)

(1) can be mitigated by either moving styles into UA sheets, exposing properties onto CSS variables onto the host and loading a SD sheet that uses them, or by implementing Shadow Parts (https://bugzilla.mozilla.org/show_bug.cgi?id=1505489).

(2) is a wontfix - we just need to wait until all the contents inside of these elements don't use XBL slotting (which could be true after menuitem, label, etc are converted (https://bugzilla.mozilla.org/show_bug.cgi?id=1500626).

Comment 14

[Brian Grinstead [:bgrins]]

It's also possible that we can just move the children into the internal box and just not try to shim anything (updating callers to append stuff into that box instead of the host, etc). That's what I did with popupnotification (https://bugzilla.mozilla.org/show_bug.cgi?id=14870650.

Comment 15

[Paul Morris [:pmorris]]

Ah, good point about it being unsymmetrical. I suppose it would be possible to also override parent/parentNode on the inserted child elements, if we wanted to continue in that direction. (Would we then need to worry about them being moved elsewhere and then parent/parentNode being wrong?)

Or just not try to shim anything.

Here's a thought. Since the plan is to just add this to MozElements and not do customElement.define yet, one option would be to defer handling dynamic insertions/deletions/etc. and just implement the simple static case for now. That would allow de-xbl work to go forward for those simpler cases (which would cover all the ones I've seen on Thunderbird so far).

(Also, I'm happy to keep working on this, but I'm also happy to hand it off if that makes more sense.)

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Paul Morris [:pmorris] from comment #15)

Or just not try to shim anything.

Here's a thought. Since the plan is to just add this to MozElements and not do customElement.define yet, one option would be to defer handling dynamic insertions/deletions/etc. and just implement the simple static case for now. That would allow de-xbl work to go forward for those simpler cases (which would cover all the ones I've seen on Thunderbird so far).

Yeah, I think that could work (basically, don't create or append any new markup for now in the base class). I think our Places popups probably have few enough consumers that we could move around children (either in the existing markup or in the CEs themselves.

Could you give that a shot? Then Alex could test against Places popups, you could test against the task-progress stuff, and we can make a decision if it will work.

Comment 17

[Paul Morris [:pmorris]]

Attachment: de-xbl-popup-4.patch

Okay, this patch does not try to shim the slotting functionality, leaving it to consumers to do slotting manually.

I tested it with the calendar task menu and calendar snooze popup. Task menu works, but snooze popup doesn't (at least I haven't gotten it to work yet).

Details: to get the menupopup behavior I use <menupopup is="custom-element-name"> rather than <custom-element-name>. That works for the task menu, but not for the snooze popup where its connectedCallback is not called. My best guess as to why is that one is a child of a <menu> (task menu, works) and the other is a child of a <button type="menu"> (snooze popup, doesn't work).

If I do <custom-element-name> then the connectedCallback is called, but menupopup behavior is missing. The menuitems are immediately visible inside the button element rather than appearing when you click the button.

Also, when I try adding { "extends": "menupopup" } to:

customElements.define("calendar-snooze-popup", MozCalendarSnoozePopup);

to try to get the connectedCallback to fire with <menupopup is="calendar-snooze-popup">, then I get an error:

JavaScript error: chrome://global/content/customElements.js, line 49: TypeError: Illegal constructor.

So this approach may work in some cases but not in others.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 9050993 [details] [diff] [review]
de-xbl-popup-4.patch

Review of attachment 9050993 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/menupopup.js
@@ +6,5 @@
>  
> +// This is loaded into all XUL windows. Wrap in a block to prevent
> +// leaking to window scope.
> +{
> +  class MozMenuPopup extends MozXULElement {

I think instead of extending MozXULElement here you should do `extends MozElementMixin(XULPopupElement)` (see also https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/toolkit/content/widgets/autocomplete-popup.js#10).

That should fix your 'illegal constructor' error.

Comment 19

[Brian Grinstead [:bgrins]]

Also, you'll have to stick with <menupopup is=""> along with { extends: "menupopup" } (and not the custom tag name) for it to get properly wired up in platform.

If you are seeing a case where connectedCallback isn't firing after comment 18, it's possible that the CE is inside of XBL anon content and you are attempting to use it before the XBL constructor finishes. If that's the case, then do customElements.upgrade(element) to force it to get connected at the top of the XBL constructor (see https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/base/content/urlbarBindings.xml#134-135 for instance).

Comment 20

[Paul Morris [:pmorris]]

Attachment: de-xbl-popup-5.patch

Thanks! Using extends MozElementMixin(XULPopupElement) did the trick. Now it is working for both of my test cases. So this is ready for Alex to try now. (Haven't used Phabricator, so not sure how to request a review, or if that's the right next step.)

Comment 21

[Brian Grinstead [:bgrins]]

A couple tests need to be updated to use <menupopup> with the xul.css change:

• https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/layout/xul/reftest/popup-explicit-size.xul#5
• https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/layout/base/tests/chrome/test_bug420499.xul#31

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 9051070 [details] [diff] [review]
de-xbl-popup-5.patch

Review of attachment 9051070 [details] [diff] [review]:
-----------------------------------------------------------------

I've done some initial testing for places-popup-base and places-popup-arrow, and it looks like they generally works using this base class. I see a couple of tests failures at https://bugzilla.mozilla.org/show_bug.cgi?id=1531870#c21 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=c467dc891e7d7b0322cde0a855a5fdc02006b5b5, but otherwise this should be ready for review.

::: toolkit/content/widgets/menupopup.js
@@ +49,5 @@
> +      while (this.childElementCount > 1) {
> +        this.scrollBox.appendChild(this.firstElementChild);
> +      }
> +
> +      this.AUTOSCROLL_INTERVAL = 25;

I think these properties would be OK to move into the constructor which allows us to not need to think about them getting reset when the node moves around in the DOM (unless if it's important that they do).

@@ +64,2 @@
>  
> +      this.addEventListener("popupshown", () => {

Can you move these listeners into a new function like `setupMenulistPopup` or something, and then guard against these listeners being added more than once (i.e. return early in that function if it's already been called).

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 9051070 [details] [diff] [review]
de-xbl-popup-5.patch

Review of attachment 9051070 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/menupopup.js
@@ +34,5 @@
> +    connectedCallback() {
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }
> +      this.appendChild(MozXULElement.parseXULToFragment(`

One more thing: I'd prefer to move this slotting behavior into a separate function that is not called by the base class (since at least for the m-c case we have custom <content> and this wouldn't be compatible with it). Assuming you do use default <content> in your extended classes, if we had a separate function you could call that from those classes connectedCallback.

Comment 24

[Paul Morris [:pmorris]]

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

Comment on attachment 9051070 [details] [diff] [review]
de-xbl-popup-5.patch

Review of attachment 9051070 [details] [diff] [review]:

I've done some initial testing for places-popup-base and places-popup-arrow,
and it looks like they generally works using this base class. I see a couple
of tests failures at
https://bugzilla.mozilla.org/show_bug.cgi?id=1531870#c21 and
https://treeherder.mozilla.org/#/
jobs?repo=try&revision=c467dc891e7d7b0322cde0a855a5fdc02006b5b5, but
otherwise this should be ready for review.

Thanks, I took a look at the tests on treeherder and didn't see any besides those two you mentioned above. I've fixed them in the patch I'm about to upload.

::: toolkit/content/widgets/menupopup.js
@@ +49,5 @@

•  while (this.childElementCount > 1) {
  

•    this.scrollBox.appendChild(this.firstElementChild);
  

•  }
  

•  this.AUTOSCROLL_INTERVAL = 25;
  

I think these properties would be OK to move into the constructor which
allows us to not need to think about them getting reset when the node moves
around in the DOM (unless if it's important that they do).

Good call, done.

@@ +64,2 @@

•  this.addEventListener("popupshown", () => {
  

Can you move these listeners into a new function like setupMenulistPopup
or something, and then guard against these listeners being added more than
once (i.e. return early in that function if it's already been called).

Done.

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

Comment on attachment 9051070 [details] [diff] [review]
de-xbl-popup-5.patch

Review of attachment 9051070 [details] [diff] [review]:

::: toolkit/content/widgets/menupopup.js
@@ +34,5 @@

• connectedCallback() {

•  if (this.delayConnectedCallback()) {
  

•    return;
  

•  }
  

•  this.appendChild(MozXULElement.parseXULToFragment(`
  

One more thing: I'd prefer to move this slotting behavior into a separate
function that is not called by the base class (since at least for the m-c
case we have custom <content> and this wouldn't be compatible with it).
Assuming you do use default <content> in your extended classes, if we had a
separate function you could call that from those classes connectedCallback.

Done. Here comes patch 6.

Comment 25

[Paul Morris [:pmorris]]

Attachment: de-xbl-popup-6.patch

I tried to set a review flag, but it says that's deprecated and to use phabricator. Not sure how to request a review on phabricator.

Comment 26

[Paul Morris [:pmorris]]

Attachment: de-xbl-popup-changes-from-5-to-6.patch

This is just the changes from patch 5 to patch 6, in case it's helpful to see those easily.

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Paul Morris [:pmorris] from comment #25)

Created attachment 9054315 [details] [diff] [review]
de-xbl-popup-6.patch

I tried to set a review flag, but it says that's deprecated and to use phabricator. Not sure how to request a review on phabricator.

The user guide is at https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html. It's preferred to use that since the reviewing and landing process is more tightly integrated with other tooling (i.e. automatic linting, being able to push to autoland). It'll be fine to handle this on splinter since you already have something up. If you plan to make more changes to m-c it would be appreciated if you took the time to set up phab (and let us know about any problems you run into, if any).

Comment 28

[Brian Grinstead [:bgrins]]

This needs a rebase around Bug 1535182. Basically, replace MozElementMixin with MozElements.MozElementMixin.

The exact error is: "JavaScript error: chrome://global/content/elements/menupopup.js, line 10: ReferenceError: MozElementMixin is not defined".

Comment 29

[Paul Morris [:pmorris]]

Attachment: Bug 1531870 - [de-xbl] convert popup binding to custom element; r=bgrins

For now, only add the MozMenuPopup base class to MozElements,
and don't define a custom element for it with
customElements.define. This is to help avoid conflicts in
de-xbl work. (See the bug for details.)

Includes a function to do 'manual slotting', moving child
elements into place. Dynamically adding, modifying, or
removing child nodes after the element is connected needs
to be handled manually as well.

Comment 30

[Paul Morris [:pmorris]]

Ah, thanks for the info on phabrictor and the heads-up about the rebase.

I've now rebased to the most recent commit and used MozElements.MozElementMixin. I've also rebased my Thunderbird work that depends on this and confirmed that it still works with this latest revision.

I went ahead and set up phabricator. The setup process was very smooth and well documented. I submitted my patch there:
https://phabricator.services.mozilla.com/D25467
I've put bgrins as the reviewer in the commit message, feel free to change that if needed.

I'm set up to do try server runs for Thunderbird but not Firefox. Let me know if I need to do a Firefox one. (Not sure how/when that fits into the review process.)

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Paul Morris [:pmorris] from comment #30)

Ah, thanks for the info on phabrictor and the heads-up about the rebase.

I've now rebased to the most recent commit and used MozElements.MozElementMixin. I've also rebased my Thunderbird work that depends on this and confirmed that it still works with this latest revision.

I went ahead and set up phabricator. The setup process was very smooth and well documented. I submitted my patch there:
https://phabricator.services.mozilla.com/D25467
I've put bgrins as the reviewer in the commit message, feel free to change that if needed.

I'm set up to do try server runs for Thunderbird but not Firefox. Let me know if I need to do a Firefox one. (Not sure how/when that fits into the review process.)

Thanks. I went ahead and did a try push on m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a69535aa8b60a988a0494211c37172cc899c1fce.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/666dcd68b023
[de-xbl] convert popup binding to custom element; r=bgrins

Comment 33

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/666dcd68b023