1499423 - Migrate the 3 treecol bindings into a Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P4)

Description

[Brian Grinstead [:bgrins]]

https://bgrins.github.io/xbl-analysis/tree/#treecol-base

I suspect we can migrate the base binding to a Custom Element (https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/TreecolBase.js), and then conditionally set the content of the element in connectedCallback based on whether the `treecol-image` class is set: https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/toolkit/content/widgets/tree.xml#1342-1353.

We'll have to take care to make sure we wire up the xbl:inherits behavior in the currently anonymous content (i.e. pass down attrs from the host down to the content both when first rendered and also when one of those attrs changes).

Comment 1

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #0)
> conditionally set the content of the element in
> connectedCallback based on whether the `treecol-image` class is set:

This looks comm-central only, so defining a subclass there and changing the comm-central consumers to use <treecol is="treecol-image"> seems like a better solution. This would have also been a valid option if this was used in mozilla-central. We didn't do this for cases like the spell-checking textbox binding because it would just increase complexity unnecessarily, but here it's the same complexity if not simpler.

> We'll have to take care to make sure we wire up the xbl:inherits behavior in
> the currently anonymous content (i.e. pass down attrs from the host down to
> the content both when first rendered and also when one of those attrs
> changes).

We can probably make this more lightweight by not propagating attributes used just for styling, like sortDirection, and rewriting the CSS. We have to continue to propagate changes to the label though.

As we found out to be usual for these conversions, be sure to review platform code that may depend on the item structure:

https://searchfox.org/mozilla-central/search?q=%3A%3Atreecol

There are also some uses of the element for generic headers outside of the <tree> element:

https://searchfox.org/mozilla-central/source/browser/components/preferences/permissions.xul#55-61

Seeing how the binding is seemingly very basic with no behavior attached, this is probably not a problem, just something to keep in mind in case we need to add glue code for any reason.

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to :Paolo Amadini from comment #1)
> (In reply to Brian Grinstead [:bgrins] from comment #0)
> > conditionally set the content of the element in
> > connectedCallback based on whether the `treecol-image` class is set:
> 
> This looks comm-central only, so defining a subclass there and changing the
> comm-central consumers to use <treecol is="treecol-image"> seems like a
> better solution. This would have also been a valid option if this was used
> in mozilla-central. We didn't do this for cases like the spell-checking
> textbox binding because it would just increase complexity unnecessarily, but
> here it's the same complexity if not simpler.
> 

Worth considering is that this bug is a stop-gap to potentially removing all these custom elements (when rewriting XUL tree layout/logic to HTML/JS), so I'm ambivalent to what approach we take in the short term.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-10-25-1157.diff

So, I've tried doing the most basic thing, and converting everything to custom elements, to start. Simplifying things comes later.

I have a few questions with this patch, inline.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 9019993 [details] [diff] [review]
remove-treecol-2018-10-25-1157.diff

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

::: toolkit/content/widgets/tree.js
@@ +53,5 @@
> +    }
> +
> +    connectedCallback() {
> +      // FIXME: MozBaseControl wasn't upgraded to a custom element yet.
> +      // super.connectedCallback()

MozBaseControl wasn't upgraded to a custom element yet, so there's no `super.connectedCallback`. Is this expected?

@@ +205,5 @@
> +      aEvent.preventDefault();
> +    }
> +  }
> +
> +  customElements.define("treecol-base", MozTreecolBase);

I was getting `this.view is undefined` errors, so I've added the following before defining the custom element:

> MozXULElement.implementCustomInterface(MozTreecolBase, [Ci.nsIDOMXULControlElement]);

Then I also pulled latest m-c and it works even without this line. Is this related at all, is there something specific that should be done here instead, "extends" maybe?

@@ +232,5 @@
> +  }
> +
> +  customElements.define("treecol-image", MozTreecolImage);
> +
> +}

I would've expected all of this to work, but the UI looks empty for all the trees that I've tested (see attached screenshot). There's no errors anywhere. Any hints on debugging?

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: empty trees

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 9019993 [details] [diff] [review]
remove-treecol-2018-10-25-1157.diff

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

::: toolkit/content/widgets/tree.js
@@ +53,5 @@
> +    }
> +
> +    connectedCallback() {
> +      // FIXME: MozBaseControl wasn't upgraded to a custom element yet.
> +      // super.connectedCallback()

This call isn't necessary (we don't implement connectedCallback on any of the base classes).

@@ +55,5 @@
> +    connectedCallback() {
> +      // FIXME: MozBaseControl wasn't upgraded to a custom element yet.
> +      // super.connectedCallback()
> +
> +      this.parentNode.parentNode._columnsDirty = true;

So XUL gets "unhappy" when you cause a layout to happen during parse.

The fix here is to do:

      if (this.delayConnectedCallback()) {
        return;
      }

at the top of the connectedCallback. If I do that it fixes the empty tree problem locally.

You can read more about that here: https://bugzilla.mozilla.org/show_bug.cgi?id=1495946#c19.

@@ +205,5 @@
> +      aEvent.preventDefault();
> +    }
> +  }
> +
> +  customElements.define("treecol-base", MozTreecolBase);

You shouldn't need to do that. The interface implementation is inherited automatically be `extends MozBaseControl` on the class.

@@ +209,5 @@
> +  customElements.define("treecol-base", MozTreecolBase);
> +
> +  class MozTreecol extends MozTreecolBase {
> +    connectedCallback() {
> +      super.connectedCallback()

if (this.delayConnectedCallback()) {
  return;
}

before anything else

@@ +210,5 @@
> +
> +  class MozTreecol extends MozTreecolBase {
> +    connectedCallback() {
> +      super.connectedCallback()
> +      this.appendChild(MozXULElement.parseXULToFragment(`

What about the case where this gets connected and disconnected? Does that ever happen? We could do: `this.textContent = ""` or similar before appending the child if so.

@@ +211,5 @@
> +  class MozTreecol extends MozTreecolBase {
> +    connectedCallback() {
> +      super.connectedCallback()
> +      this.appendChild(MozXULElement.parseXULToFragment(`
> +        <label class="treecol-text" inherits="crop,value=label" flex="1" crop="right"></label>

So we need to actually "inherit" the attributes (copy them from the host element onto the child label / image). We are working on a shared mechanism for that in https://phabricator.services.mozilla.com/D9843, but in the meantime as a sanity check to make sure it's working, you could do:

this.label = this.querySelector(".treecol-text");
this.label.setAttribute("value", this.getAttribute("label");
... etc

@@ +221,5 @@
> +
> +  customElements.define("treecol", MozTreecol);
> +
> +  class MozTreecolImage extends MozTreecolBase {
> +    connectedCallback() {

if (this.delayConnectedCallback()) {
  return;
}

before anything else

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> @@ +55,5 @@
> > +    connectedCallback() {
> > +      // FIXME: MozBaseControl wasn't upgraded to a custom element yet.
> > +      // super.connectedCallback()
> > +
> > +      this.parentNode.parentNode._columnsDirty = true;
> 
> So XUL gets "unhappy" when you cause a layout to happen during parse.
> 

Ah, that's very interesting, than's for pointing that out! Is there a consolidated place for all these gotchas we encounter when de-XBLing? If not, it would be worth writing some simple guidelines/documentation.

> @@ +211,5 @@
> > +  class MozTreecol extends MozTreecolBase {
> > +    connectedCallback() {
> > +      super.connectedCallback()
> > +      this.appendChild(MozXULElement.parseXULToFragment(`
> > +        <label class="treecol-text" inherits="crop,value=label" flex="1" crop="right"></label>
> 
> So we need to actually "inherit" the attributes (copy them from the host
> element onto the child label / image). We are working on a shared mechanism
> for that in https://phabricator.services.mozilla.com/D9843, but in the
> meantime as a sanity check to make sure it's working, you could do:
> 
> this.label = this.querySelector(".treecol-text");
> this.label.setAttribute("value", this.getAttribute("label");
> ... etc


This is pretty weird though, because the crop attribute is both inherited and set directly on the element. I guess `crop="right"` overrides the inherited value?

Agreed on inheriting "value", thanks for pointing that out!

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-10-26-0903.diff

This fixes all issues.

Brian, I don't have particularly strong opinions on whether or not we should fold all these 3 custom elements into a single one, or keep them separate similarly to what comment 1 suggests. I suspect it won't matter in the long run though, as this is a stop-gap to simplify things before proceeding with the rewrite. Ok not deal with it now?

Comment 9

[Victor Porof [:vporof][:vp]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6faaba39136367d77248c2521bb7d53a4d1c7502

Comment 10

[Tim Nguyen :ntim]

Can treecol-image be removed first ? Paolo mentions it’s unused in comment 1.

Comment 11

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-10-26-1508.diff

Fixed a test.

Comment 12

[Victor Porof [:vporof][:vp]]

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

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #7)
> This is pretty weird though, because the crop attribute is both inherited
> and set directly on the element. I guess `crop="right"` overrides the
> inherited value?

The "inherits" attribute is a XBL-only feature. So anything that shows up in the markup as a result of the converter needs to be removed from the markup and wired up manually (we're working on a helper function to make that easier - the current version of that is in a patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1455433).

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Victor Porof [:vporof][:vp] from comment #7)
> > This is pretty weird though, because the crop attribute is both inherited
> > and set directly on the element. I guess `crop="right"` overrides the
> > inherited value?
> 
> The "inherits" attribute is a XBL-only feature. So anything that shows up in
> the markup as a result of the converter needs to be removed from the markup
> and wired up manually (we're working on a helper function to make that
> easier - the current version of that is in a patch at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1455433).

There's both inherits crop, and explicit crop defined in there. I don't really see how we could implement both with get/set attribute.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 9020330 [details] [diff] [review]
remove-treecol-2018-10-26-1508.diff

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

::: toolkit/content/widgets/tree.js
@@ +51,5 @@
> +      });
> +
> +    }
> +
> +    connectedCallback() {

Given that this doesn't actually bind to any element, I think I'd rename this function to markTreeDirty() or similar, and then drop the delayConnectedCallback (and call it directly from extended classes instead of super.connectedCallback())

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #14)
> There's both inherits crop, and explicit crop defined in there. I don't
> really see how we could implement both with get/set attribute.

Oh yeah, I see what you are saying now. I wasn't actually sure how XBL handles this so I checked. It appears to keep the explicit setting unless if the [crop] attr is set on the parent, at which point it gets overridden. Then if you remove the [crop] attr on the parent the explicit value doesn't come back.

TBH I'm not sure if this is worth keeping - from a quick search [crop] on treecol doesn't appear to be used: https://searchfox.org/mozilla-central/search?q=%3Ctreecol.*crop&regexp=true&path=. That said, we could conditionally call the helper function only if the attr exists on the parent. For example (based on the helper fn at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1455433&attachment=9020261):

```
this.inheritAttributes(this.label, ["value=label"]);

if (this.hasAttribute("crop")) {
  this.inheritAttributes(this.label, ["crop"]
}
```

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 9020330 [details] [diff] [review]
remove-treecol-2018-10-26-1508.diff

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

This is looking good. I'll do a talos push to make sure we aren't hitting regressions due to XBL construction on parent bindings.

::: toolkit/content/widgets/tree.js
@@ +222,5 @@
> +        <label class="treecol-text" flex="1" crop="right"></label>
> +        <image class="treecol-sortdirection"></image>
> +      `));
> +
> +      let label = this.querySelector(".treecol-text");

We need to also make these attrs get set when the relevant attr changes on the host element. The way we've typically handled this is:

1: Extract the attr setting out into a helper function
2: Call (1) from connectedCallback()
3: Wire up attributeChangedCallback / observedAttributes and call (1) from there (if `this.isConnectedAndReady`).

Comment 18

[:Paolo Amadini]

Comment on attachment 9020330 [details] [diff] [review]
remove-treecol-2018-10-26-1508.diff

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

::: toolkit/content/widgets/tree.js
@@ +251,5 @@
> +      image.setAttribute("src", this.getAttribute("src"));
> +    }
> +  }
> +
> +  customElements.define("treecol-image", MozTreecolImage);

Oh, I didn't realize in comment 1 that both these bindings inherited from a base class - I thought they were much simpler and had only content and not other behavior.

I would definitely recommend to just make one class for "treecol", and removing the "treecol-image" binding, that can be trivially reimplemented in comm-central. The inherited binding would have to be defined using the special "is" attribute, because assigning it to a different tag name ("treecol-image") would require the styling to be duplicated. Anyways, that special registration would be necessary in comm-central only.

The elements being used for "richlistbox" instead of "tree" might be tackled in a follow-up, let's just make sure that they still work before landing this.

Comment 19

[Brian Grinstead [:bgrins]]

Seeing a few talos regressions at https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f71131eff43113f0f8c70877b4906c368c9d24af&newProject=try&newRevision=f719afaffeeb1345bf77714134018c625c2cf7b4&framework=1&showOnlyImportant=1. I think that's probably either because of us constructing the DOM / copying attrs etc, or because we are forcing construction of the xbl binding for the parent due to JS access in connectedCallback. If I had to guess it would be the latter.

If so, we could let the tree tell us when it's OK to render in the case where it's not yet constructed (through its own XBL <constructor>). It may get a little tricky, but it should be doable. Let's first get previous comments addressed and then we can sit down together and figure out what we need to do for perf.

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-10-29-1008.diff

Addressed comments:

1. Removed treecol-image as per comment 18.
2. Properly handling `inherits=crop` and `crop=right` as per comment 16.
3. Renamed top-level `connectedCallback` to `markTreeDirty` as per comment 15.
4. Handling attribute updates via `attributeChangedCallback` and `observedAttributes` as per comment 17.

Didn't do anything for talos yet.

Comment 21

[Victor Porof [:vporof][:vp]]

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

Comment 22

[Richard Marti (:Paenglab)]

Adding Arshad so he knows that treecol-image will be removed and depending comment 18 that it can be trivially reimplemented in comm-central.

Comment 23

[Tim Nguyen :ntim]

Now that treecol-image is removed, I believe the treecol CE can be folded with treecol-base.

Comment 24

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-10-29-1513.diff

Fixed some test failures.

See comment 20 for changes.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=8259bcd0b9d9b1242aa08a98a8520174f5e9c8b4&selectedJob=208330400

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 9020761 [details] [diff] [review]
remove-treecol-2018-10-29-1513.diff

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

Looking really good, thanks Victor! Clearing review while we sort out perf and wait for the inherits helper function to land.

::: browser/components/preferences/in-content/tests/browser_permissions_dialog.js
@@ +15,4 @@
>  function checkPermissionItem(origin, state) {
>    let doc = sitePermissionsDialog.document;
>  
> +  let label = doc.getElementsByTagName("label")[2];

I wonder if there's a better way to fetch the label for this - is there maybe a classname or attribute on this that we could use for querySelector

::: toolkit/content/widgets/tree.js
@@ +7,5 @@
> +  // This is loaded into all XUL windows. Wrap in a block to prevent
> +  // leaking to window scope.
> +  {
> +
> +  class MozTreecolBase extends MozBaseControl {

I agree with Comment 23 - let's fold this together with MozTreecol since we aren't subclassing it more than once anymore.

@@ +218,5 @@
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }
> +
> +      super.markTreeDirty();

Nit: you don't need to use `super` here since we don't override this function, you can use `this`.

I think the first thing I'd try for perf is to not make this call if we are running as part of the original connection (i.e. if delayConnectedCallback() was true earlier). Because the tree defaults to having dirty columns (so I think this is only really needed if this is added to a tree that's already been constructed): https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/toolkit/content/widgets/tree.xml#118.

Let me do a talos push with this line just commented out to see if it has an effect.

@@ +221,5 @@
> +
> +      super.markTreeDirty();
> +
> +      this.textContent = "";
> +      this.appendChild(MozXULElement.parseXULToFragment(`

Can we make a getter something like: 

```
get content() {
return MozXULElement.parseXULToFragment(`
  <label class="treecol-text" flex="1" crop="right"></label>
  <image class="treecol-sortdirection"></image>
`);
}
```

That will make it easier to override this with a subclass for Arshad - I expect it could then be done with something like:

customElements.define("treecol-image", class TreeColImg extends customElement.get("treecol") {
  get content() {...}
  static get observedAttributes() {...}
  _updateAttributes() {...}
}, { extends: "treecol" });

Comment 26

[Brian Grinstead [:bgrins]]

Talos looked pretty good when skipping the call to `markTreeDirty` in the case where we are running delayed connected callback (i.e. when tree is parsed). We discussed setting a bit on the element when delayConnectedCallback is called and unsetting it after connectedCallback is manually called in DOMContentLoaded as a way to tell the element to skip it.

Comment 27

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-11-01-1142.diff

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 9021778 [details] [diff] [review]
remove-treecol-2018-11-01-1142.diff

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

Thanks!

::: toolkit/content/customElements.js
@@ +37,2 @@
>          element.connectedCallback();
> +        element.isRunningDelayedConnectedCallback = false;

I'd move the false setting outside of the try/catch to make sure we don't get stuck with this set to true if the connectedCallback threw.

Comment 29

[Victor Porof [:vporof][:vp]]

Good point, will do!

Comment 30

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-11-01-1615.diff

Comment 31

[Victor Porof [:vporof][:vp]]

Comment on attachment 9021844 [details] [diff] [review]
remove-treecol-2018-11-01-1615.diff

Whoops wrong patch.

Comment 32

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecol-2018-11-01-1618.diff

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4548dcc2c7f6
Migrate the 3 treecol bindings into a Custom Element, r=bgrins

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4548dcc2c7f6