stencil icon indicating copy to clipboard operation
stencil copied to clipboard

feat: Fallthrough attributes

Open Timmitry opened this issue 2 years ago • 7 comments
trafficstars

Prerequisites

Describe the Feature Request

Add support for fallthrough attributes, similar to what Vue.js does.

Describe the Use Case

Let's say we implement a custom <my-button>, which basically just wraps a native <button>-Element. We want to define some Props that are important for e.g. styling (like variant, which could be primary or secondary). However, there are many attributes that can be set on a native button that we have to consider - take, for example, the disabled-attribute.

Currently, the only way to enable setting disabled on the custom button would be to add it as a Prop, and pass this prop to the <button> in the render method.

Describe Preferred Solution

When using a stencil component, all attributes that are passed which are not explicitly declared on the component will be passed to the root element of the render function.

If another element should receive the fallthrough attributes, it could be marked in some way.

If there are multiple root elements, no element automatically receives the attributes.

Describe Alternatives

Make the feature opt-in, and only enable fallthrough attributes when explicitly declaring an element that should receive the fallthrough attributes.

Related Code

import { Component, h, Prop } from '@stencil/core';

@Component({
  tag: 'my-button',
  shadow: true,
})
export class MyButton {
  @Prop() variant: "primary" | "secondary";
  @Prop() type: string = "button";
  @Prop() disabled: boolean = false;

  render() {
    return (
      <button
        type={this.type}
        data-variant={this.variant}
        disabled={this.disabled}
      >
        <slot></slot>
      </button>
    )
  }
}

Additional Information

No response

Timmitry avatar Nov 08 '23 14:11 Timmitry

Ionic has a helper for this and the attributes are defined (opt-in) on a component-basis.

https://github.com/ionic-team/ionic-framework/blob/main/core/src/utils/helpers.ts#L92

This is how I would do it, however, I'm still not sure how it would work with some elements and attributes. Not every attribute of a button is valid on a custom element with a generic HTML Element interface. For things like ARIA, it seems fine.

dgibson666 avatar Nov 08 '23 21:11 dgibson666

@dgibson666 Thanks for your Input! The inheritAttributes-method you linked would be the allowlist way, where I still have to specify each attribute that should be inherited. This is a lot better than specifying many props and assigning them to the element as attributes.

However, I'd still like to consider the approach where simply all extra-attributes are passed down. This would simplify things a lot, as one wouldn't have to copy the lengthy list of allowed attributes for a button in the code, just to miss the one that some user wanted.

Not every attribute of a button is valid on a custom element with a generic HTML Element interface.

Hm, do you know of any restrictions regarding attributes on custom elements? I always thought that you can put all attributes on a custom element, as you are the one controlling the behaviour. Be it, disabled, title, type, data-xyz, aria-xyz, ...

Timmitry avatar Nov 09 '23 05:11 Timmitry

Hm, do you know of any restrictions regarding attributes on custom elements? I always thought that you can put all attributes on a custom element, as you are the one controlling the behaviour. Be it, disabled, title, type, data-xyz, aria-xyz, ...

@Timmitry I see what you're saying. If you could spread the ...rest onto the rendered element (or even manipulate it and split it onto multiple elements) that could be convenient in some cases.

But the typing issues are still present. A custom element's valid attributes are inherited from the HTMLElement interface (which extends the Element interface) - basically all "global attributes", which includes title, style, class, data-* and aria-*, etc. - plus its defined props. So in your button example, if you wanted to pass down type or formaction to the rendered button tag, which aren't global but part of the HTMLButtonElement interface, they should be seen as invalid on the component - if not defined as props - when type-checked. You "control the behavior" by defining props. I'm not sure if there's any sort of typescript workaround for that.

Comparing to the Ionic solution, you also need to remove those extra attributes from the host element, because global attributes are valid on any element, but keeping them on the host custom element when it's not the intention can cause issues too (e.g., role, aria-*).

dgibson666 avatar Nov 09 '23 19:11 dgibson666

@dgibson666 Unfortunately the Ionic helper solution does not work for inherited attributes that are updated dynamically. This is a killer for aria states such as "aria-expanded" and "aria-pressed". There's even a bug report in Ionic about it:

https://github.com/ionic-team/ionic-framework/issues/27477

@Timmitry If you are desperate you can try a mutation observer, for the global attributes at least. The following code works in a vanilla javascript environment, but I have not tried it out in React or any other framework, so be warned...

private attributeObserver: MutationObserver;

private oldAttrbutes: {[key: string]: string} = {}

private newAttributes: {[key: string]: string} = {}

componentWillLoad() {
  const ariaAttributes = Array.from(this.hostEl.attributes).map(attr => attr.name).filter(name => name.startsWith("aria-"))

  ariaAttributes.forEach(attributeName => {
    const newValue = this.hostEl.getAttribute(attributeName) // Also check the new value if you wish

    this.newAttributes = {...this.newAttributes, [attributeName]: newValue}
    
    this.hostEl.removeAttribute(attributeName)
  })
}

componentDidLoad() {
  const callback = (mutationList: MutationRecord[]) => {
    mutationList.forEach((mutation) => {
      const { attributeName, oldValue } = mutation

      if (attributeName.startsWith("aria-")) {
        const newValue = this.hostEl.getAttribute(attributeName) // Also check the new value if you wish

        this.oldAttrbutes = {...this.newAttributes, [attributeName]: oldValue}
        this.newAttributes = {...this.newAttributes, [attributeName]: newValue}
        
        this.hostEl.removeAttribute(attributeName)
        
        // Pause the observer and break the loop
        this.attributeObserver.disconnect()

        this.attributeObserver.observe(this.hostEl, {
          attributes: true,
          attributeOldValue: true
        })
      }
    });

    if (this.oldAttrbutes !== this.newAttributes) {
      forceUpdate(this)
    }
  };

  this.attributeObserver = new MutationObserver(callback);

  this.attributeObserver.observe(this.hostEl, {
    attributes: true,
    attributeOldValue: true
  });
}

disconnectedCallback() {
  this.attributeObserver.disconnect();
}

// Use new attributes in render 

After much discussion, my team and I have decided to add our own attributes such as "label", "description" and "expanded" to the host element so we can control (a la @dgibson666) and perhaps correct for how the attributes are used.

That said, the whole thing has been a big headache and we still haven't figured out what to do with data attributes, which can also change. This feature has my vote for any attribute that would be allowed on a host HTMLElement.

elaineoliver avatar Feb 21 '24 11:02 elaineoliver

@elaineoliver I just recently built a button component, which can be used as a control, and added expanded and pressed Boolean props to it. If you want robust reactivity, this is the job of props, and anything resembling statefullness should be integrated into the component API and lifecycle, IMO. I think you did the right thing.

Adding generic attributes to the host (which may be valid there or not) and hoping to pass them through to a rendered element should really be an edge-case. I can see how just giving someone the option might raise the expectation that they are reactive as well but that would seem to conflict with removing them from the host element as I mentioned above. It seems like a big can of worms to me that can lead to many cases of unintended consequences. I'm curious to see how/if the Ionic team handles it.

dgibson666 avatar Feb 21 '24 14:02 dgibson666

Thanks @dgibson666, I hope it's the right decision.

For anyone else reading this thread who's not quite ready to climb out of the rabbit hole, here's what some other frameworks are doing:

Porche (Stencil): Gives components a single attribute "aria" which accepts a json string, or just json if you're using a framework. Porsche Design System button element in GitHub

Needless to say there's a lot of processing and type checking of said string behind the scenes before the attributes are applied to the target button, or whatever. My team is considering this solution for users who want to apply data attributes to elements inside the shadow dom.

Material Web (Lit): Turns aria attributes into reactive properties. Material Web Components aria helper function in GitHub

Sounds nice, but they are getting around the duplicate global attribute problem by setting the role of the host element to "presentation." Little problem, according to the spec:

Browsers ignore role="presentation", and it therefore has no effect, if either of the following are true about the element to which it is applied:

  1. The element is focusable, e.g. it is natively focusable like an HTML link or input, or it has a tabindex attribute.

  2. The element has any global ARIA states and properties, e.g., aria-label.

https://www.w3.org/WAI/ARIA/apg/practices/hiding-semantics/

I only have Voice Over on the Mac, but I can confirm that given a component with an aria-label on the host element and an aria-label on a focussable element in the shadow dom, Voice Over will read the label twice. Not cool.

And finally:

Adobe Spectrum (Lit): Appears to be reverse engineering host elements into buttons, links and other things. Spectrum Web Components button component in GitHub

So problem solved?

elaineoliver avatar Feb 23 '24 09:02 elaineoliver