1545824 - Support attribute inheritance for both light DOM and shadow DOM within a single chrome custom element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement, P3)

Description

[Paul Morris [:pmorris]]

The following currently does not work but probably should:

Extend a custom element that has a shadowDOM/shadowRoot (like 'tree') to create another custom element (like 'calendar-list-tree'), and use this.initializeAttributeInheritance so attributes in your custom element are inherited. The attributes are not inherited as expected.

The code currently will search for elements with inherited attributes that are part of the shadowRoot/shadowDOM if it exists, and not search for them in the non-shadowRoot child elements.

I'll upload a patch to demonstrate the issue and suggest a possible fix.

I ran into this while working on bug 1504416. I can work around it by manually inheriting attributes in connectedCallback, but wanted to also file a bug.

Comment 1

[Paul Morris [:pmorris]]

Attachment: attribute-inheritance-shadow-dom-0.patch

This patch shows the problem and a potential way to fix it.

Comment 2

[Brian Grinstead [:bgrins]]

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

The following currently does not work but probably should:

Extend a custom element that has a shadowDOM/shadowRoot (like 'tree') to create another custom element (like 'calendar-list-tree'), and use this.initializeAttributeInheritance so attributes in your custom element are inherited. The attributes are not inherited as expected.

So the does the calendar-list-tree create it's own light DOM and then rely on the tree's Shadow DOM to slot it around?

Comment 3

[Paul Morris [:pmorris]]

Ah, would be better if I had a patch up for the calendar-list-tree work. Should have that up sooner than later.

Basically in calendar-list-tree's conncetedCallback I call this.appendChild(MozXULElement.parseXULToFragment passing a XUL string with <treecols> and <treechildren> elements.

That works as expected, so I haven't looked into the specifics of how the slotting works. I haven't done anything with slotting in calendar-list-tree code.

Comment 4

[Brian Grinstead [:bgrins]]

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

Ah, would be better if I had a patch up for the calendar-list-tree work. Should have that up sooner than later.

Basically in calendar-list-tree's conncetedCallback I call this.appendChild(MozXULElement.parseXULToFragment passing a XUL string with <treecols> and <treechildren> elements.

That works as expected, so I haven't looked into the specifics of how the slotting works. I haven't done anything with slotting in calendar-list-tree code.

If we are overriding inheritedAttributes on the subclassed element then that would break the attribute inheritance on the base class (unless if you called super.inheritedAttributes, but then you'd need some way to distinguish which should apply to the shadow root and which should apply to the light DOM).

I think Alex had an idea before where you could implement a getElement or similar function on the class and then you would geta chance to look at the selector and decide what (if any) element to return. So it would replace this https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/toolkit/content/customElements.js#289-290.

Something like:

class MozXULElement {
  getElement(selector) {
    let parent = this.shadowRoot || this;
    return parent.querySelector(selector);
  }
}

class FooExtended extends MozXULElement {
  static get inheritedAttributes() {
    return { ".one": "attr" };
  }
}

class FooExtended extends customElements.get("foo") {
  static get inheritedAttributes() {
    return Object.assign({}, super.inheritedAttributes(), {
      ".two": "attr",
    });
  }
  getElement(selector) {
    if (selector == ".two") {
      return this.querySelector(selector)
    } else {
      return super.getElement(selector);
    }
  }
}

Comment 5

[Paul Morris [:pmorris]]

Huh, yeah, I see how that would work.

And yeah, I'm using super.inheritedAttributes when overriding inheritedAttributes, like so:

static get inheritedAttributes() {
  return {
    // Include inherited attributes from MozTree parent class,
    // otherwise scrollbars are not hidden properly.
    ...super.inheritedAttributes,
    ".treecols": "hideheader"

    /* etc... */
  };
}

Comment 6

[Brian Grinstead [:bgrins]]

OK, let's aim to do something like the overridden function then (maybe called something like getElementForInheritance instead so it's not confusing to consumers). Somehow detecting which of the selectors are coming from the base and using shadow DOM for them and then using light DOM for the rest seems hard to get right for all cases.

Paul, is that something you could take? Note there's a tangentially related bug with attribute inheritance that I think will block this one which was noticed during Bug 1538983 and can be fixed with https://hg.mozilla.org/try/diff/5060ce1b1ddd/toolkit/content/customElements.js. Planning to handle that in another bug.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 9059556 [details] [diff] [review]
attribute-inheritance-shadow-dom-0.patch

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=1545824#c6

Comment 8

[Paul Morris [:pmorris]]

Hmm, I was thinking about this some more, trying to come up with a simpler way that would avoid the extra handling of a getElementForInheritance function. It is trickier than it looks.

One way would be to split up inheritedAttributes and define them as two groups, one for shadow elements and one for light elements (like inheritedAttributes and inheritedAttributesShadow or similar names). Then initializeInheritedAttributes would know where to look for each set of them.

While that would solve this particular issue, I'm not sure if all the changes it would require would make sense in the broader context and given how this code is organized... (I'm not as familiar with it.)

In any case this is something I'd be willing to take on, if that makes sense for how we decide to solve it. I'm not really blocked on this, and I'll want to go ahead and finish my calendar-list-tree work while I'm close to having it ready for review. (Just using the manual attribute inheritance work-around.) That will also give some time for that other blocking bug to be fixed.

Comment 9

[Brian Grinstead [:bgrins]]

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

Hmm, I was thinking about this some more, trying to come up with a simpler way that would avoid the extra handling of a getElementForInheritance function. It is trickier than it looks.

One way would be to split up inheritedAttributes and define them as two groups, one for shadow elements and one for light elements (like inheritedAttributes and inheritedAttributesShadow or similar names). Then initializeInheritedAttributes would know where to look for each set of them.

While that would solve this particular issue, I'm not sure if all the changes it would require would make sense in the broader context and given how this code is organized... (I'm not as familiar with it.)

Yeah, that's a reasonable alternative and seems like it would be a relatively small change. I think I still prefer having something like an overridden getElementForInheritance function since it's more flexible (for instance, you could return elements that haven't even been appended in the document yet if you wanted to make some DOM in the constructor but then wait to lazily insert it until something happens). That could get kind of confusing since it's not really a "selector" as the key anymore but it would be up to the individual component to choose how complicated it wants to get. Also, we haven't hit this particular limitation yet so I'm speculating a bit. Going to needinfo for some more opinions.

In any case this is something I'd be willing to take on, if that makes sense for how we decide to solve it. I'm not really blocked on this, and I'll want to go ahead and finish my calendar-list-tree work while I'm close to having it ready for review. (Just using the manual attribute inheritance work-around.) That will also give some time for that other blocking bug to be fixed.

Makes sense.

Comment 10

[Andrew Swan [:aswan]]

My gut reaction is a slight preference for the suggestion in comment 8 (separate inheritance lists for shadow and non-shadow elements), just to avoid an extra layer of indirection that makes understanding how inheritance works incrementally more complicated.
But that said, its not a strong preference, I wouldn't object to the suggestion in comment 9 (getElementForInheritance).

Comment 11

[alexander :surkov (:asurkov)]

I like an approach of having a function that returns an element by a selector, because it's real flexible: it can handle everything not only shadow or explicit CSS selectors, for example, it can operate on properties:

class El {
static get inheritedAttributes() {
"boo": "label",
}
getElementForAttrInheritance(selector) {
return this[selector];
}
get boo() {
return this._boo;
}
}

the only issue I could think of is getElementForAttrInheritance function discoverability: you simply have to be aware of its existence. I suppose some MDN docs can help with it though. I'm not sure though I can see the point why this function is trickier than it looks (comment #8), since it's just a JS wrapper around querySelector().

I suppose I can live with inheritedAttributes/inheritedAttributesShadow approach, as its downsides I could name it's less flexible and more bulky/complicated since it you have to remember to use a different function when you deal with shadow DOM content, which is sort of implicit, but again MDN docs might help to mitigate this.

Comment 12

[Paul Morris [:pmorris]]

Attachment: Bug 1545824 - Implement a getElementForAttrInheritance function

Comment 13

[Paul Morris [:pmorris]]

I've uploaded a patch that implements a getElementForAttrInheritance function as discussed here. Looks like I should have requested review. (via the commit message? I'm unfamiliar with Phabricator workflows.) I just tried to add a review request for :bgrins via bugzilla but the "?" option wasn't there. Doing a needinfo instead.

Comment 14

[Andrew Swan [:aswan]]

Brian is unavailable for a few weeks, I've flagged myself for review on Phabricator (for future reference, you can do that with either "Edit Revision" near the top of the page or with "Change Reviewers" in the actions menu at the bottom of the page)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/528d12607600
Implement a getElementForAttrInheritance function r=aswan

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/528d12607600