Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attr fallthrough behavior #137

Closed
wants to merge 11 commits into from
Closed

Attr fallthrough behavior #137

wants to merge 11 commits into from

Conversation

yyx990803
Copy link
Member

This proposal is a replacement of #92.

  • Implicit fallthrough now by default only applies for a whitelist of attributes (class, style, event listeners, a11y attributes, and data attributes).

  • Implicit fallthrough now works consistently for both stateful and functional components (as long as the component has a single root node).

  • this.$attrs now contains everything passed to the component minus those explicitly declared as props, including class, style, and listeners. (this.$listeners is removed)

The main reason for the revision is the conflict between Optional Props Declaration and the implicit fallthrough behavior. See detailed explanation in the Motivation section.

Full Proposal, Rendered

@yyx990803 yyx990803 added breaking change This RFC contains breaking changes or deprecations of old API. core 3.x This RFC only targets 3.0 and above labels Feb 28, 2020
yyx990803 added a commit to vuejs/core that referenced this pull request Feb 28, 2020
BREAKING CHANGE: adjust attr fallthrough behavior

    Updated per pending RFC vuejs/rfcs#137

    - Implicit fallthrough now by default only applies for a whitelist
      of attributes (class, style, event listeners, a11y attributes, and
      data attributes).

    - Fallthrough is now applied regardless of whether the component has
      explicitly declared props. (close #749)
@CyberAP
Copy link
Contributor

CyberAP commented Feb 29, 2020

Would it be possible to align with the HTML spec and apply implicit fallthrough for all the Global attributes?

The problem with a custom whitelist is that it's Vue-specific. You'll have to memorize it and there's no way to opt-out of this. Otherwise you'll encounter weird behaviour and won't be able to quickly tell what went wrong.

For all the people who are familiar with HTML, Global attributes are a known thing and they know how they behave. I think it can be confusing for people to see when style is falling through, but lang or tabindex are not.

have a low chance of being used as a component prop (e.g. id is not a good candidate)

Could you elaborate a bit more why this rule is important?
In a case where id is a prop you could just use another prop to set an id attribute. That might not be consistent with other id usage across the whole project, but it's up to the component's authors to make a convenient API. It's similar to how you can't declare a key prop even though it may make sense for a particular component. The same thing applies to a title attribute. If you need both an attribute and a prop you'll need to declare both. If using title as an attribute doesn't work as expected and you have no control over the component there's also an option to wrap your component and set an attribute there, which would always work.

The only problem I see with this are is and slot attributes, which serve their own purpose in Vue. Since they're related to web-components and won't make sense as a fallthrough attributes for a Vue component I suppose these are the only exceptions that should exist in the list.

And lastly adding structured data markup would be much easier with this change since it's not tied to component internals.

@posva
Copy link
Member

posva commented Feb 29, 2020

You'll have to memorize it and there's no way to opt-out of this

You can opt-out with intheritAttrs. It's a very short list so it can be memorised. Global attributes, in comparison, is way longer and harder to memorise.
I'm also positive that most people working with Vue (even those familiar with HTML) do not know the whole list of global attributes. Also, taking into consideration only people familiar with HTML is not taking into consideration the different people using Vue.

@CyberAP
Copy link
Contributor

CyberAP commented Feb 29, 2020

I'm also positive that most people working with Vue (even those familiar with HTML) do not know the whole list of global attributes.

You don't have to know the whole list by heart, but you'll be confident that everything that is global in HTML is global in Vue as well. "A known thing" was not about knowing every little detail about this list. It was about being familiar with the concept of it. Everyone who worked with HTML knows how a class or id behaves and that you can assign it to every single element in your markup. That's very confusing that this behaviour works for class and does not apply to tabindex or any other global attribute.

Also, taking into consideration only people familiar with HTML is not taking into consideration the different people using Vue.

How are people working with class or style in Vue different from those working with HTML only? If you don't know HTML you'll have a hard time working with those as well, not considering the rest of the list.

@posva
Copy link
Member

posva commented Feb 29, 2020

You don't have to know the whole list by heart, but you'll be confident that everything that is global in HTML is global in Vue as we

Yes you do need to know it, because you could use a global property without knowing it is one, ending up in unexpected behavior when using optional props. And it's very difficult to debug.

How are people working with class or style in Vue different from those working with HTML only? If you don't know HTML you'll have a hard time working with those as well, not considering the rest of the list.

One thing is working with HTML and something else is being familiar with it. The point is the public varies and you cannot expect them to know all of the global attributes.

I hope that makes more sense on why using the whole list of global attributes is not a good idea

@yyx990803
Copy link
Member Author

yyx990803 commented Feb 29, 2020

The potential issue with including a global attribute like id is that when a user authors a component like this:

const Foo = props => h('div', `id: ${props.id}`)

And didn't expect id to be also set as an attribute on the div. The bigger the whitelist is, the more this could happen. The undesired behavior is unrelated to their intention (i.e. the user may not even be aware of the fallthrough behavior), which makes it more difficult to notice or pinpoint why it happened.

On the contrary, when users rely on the fallthrough behavior, they are always intentionally doing so. If they expect an attribute to fallthrough but it didn't, it is easier for them to notice and figure out why. It is also technically possible for us to warn against an attribute that failed to fallthrough and wasn't accessed during render.

Some additional points:

  • The global attributes list is much harder to remember than the proposed whitelist. The whitelist is also intentionally designed to minimize memorization efforts:

    • Most users are used to using class and style for styling purposes and will never use them as prop names.
    • data-x, aria-x and @xxx are very self-evident and don't require much memorization efforts, and again they are unlikely to be used as prop names.
    • The only special case is role, but if the developer knows about a11y and ARIA then this makes sense too just like aria-x.
  • The list of global attributes is not finite. There may be more global attributes added to the spec in the future, and expanding the whitelist is technically a breaking change for Vue.

  • Overall the list should seek a balance between "pragmatic use cases" and "encapsulating behavior inside the component". Some global attributes like contenteditable completely alters the behavior of everything inside the element and it doesn't make much sense for a component to allow that.

@CyberAP
Copy link
Contributor

CyberAP commented Feb 29, 2020

First of all, thanks for the detailed answer!

The potential issue with including a global attribute like id is that when a user authors a component like this:

My assumption is that if it's requested like props.id then it shouldn't be there in the first place when the props are not declared. I understand that the new props behaviour has been adjusted already, but it's not intuitive anyway, doesn't matter whether a custom whitelist or an HTML one is used. props.id is a bad practice in itself without using props declaration because it creates a lot of ambiguity on how a component should work and leads to issues such as this one.
It's also strange that class is considered ok for styling purposes, but ids are not. While in fact they both could serve styling (or scripting) purposes.

The global attributes list is much harder to remember than the proposed whitelist.

I would like to note that you'll have to memorize this list only in case you have never used global properties. If you want to use a global property chances are you already know it's global. With the whitelist you'll have to limit the global properties you already know to a Vue-specific list.

The only special case is role, but if the developer knows about a11y and ARIA then this makes sense too just like aria-x.

This to me is as confusing as an id example, since ARIA is a very specific knowledge and I suspect there are much less people who know about ARIA rather than about Global attributes when working with HTML. But I might be wrong here, couldn't find any data on that unfortunately since ARIA is not a part of any frontend surveys nowadays.

The list of global attributes is not finite. There may be more global attributes added to the spec in the future, and expanding the whitelist is technically a breaking change for Vue.

In the same sense any new built-in HTML element is a breaking change for Vue, but that doesn't prevent us from using those in templates.
I'd like to note that W3C has an established review and release process so Vue users could be notified at least half a year in advance of the upcoming changes.

Overall the list should seek a balance between "pragmatic use cases" and "encapsulating behavior inside the component". Some global attributes like contenteditable completely alters the behavior of everything inside the element and it doesn't make much sense for a component to allow that.

I agree that some properties just don't make sense on a component at a first glance, but even with contenteditable I can imagine a usecase where we have a user generated content that we'd like to alter similar to a design mode and then save.


Even if the idea of Global attributes doesn't get any traction I'd like to propose some attributes that I think should have a fallthrough behaviour:

  • dir to use with user-generated content in an RTL language
  • Microdata attributes (structured data markup) to separate components markup and structured data markup:
    • itemprop
    • itemid
    • itemref
    • itemscope
    • itemtype
  • lang to use with user-generated content in a different language from the document
  • tabindex to allow, disable or contain focus in a component

@yyx990803
Copy link
Member Author

yyx990803 commented Mar 1, 2020

My assumption is that if it's requested like props.id then it shouldn't be there in the first place when the props are not declared.

The whole reason we are introducing the whitelist is to support such usage (as proposed in Optional Props Declaration).

props.id is a bad practice in itself without using props declaration because it creates a lot of ambiguity on how a component should work

It is useful for simple one-off components. React works like that without much issue. Also with TypeScript you can annotate the props type without having to use the runtime declaration. In fact, TypeScript users will likely prefer annotating prop types with TS interfaces rather than the runtime JavaScript options.

It's also strange that class is considered ok for styling purposes, but ids are not. While in fact they both could serve styling (or scripting) purposes.

Because in the context of styling components, class is almost always a better choice than id. class and style can be merged, but allowing id to fallthrough has a chance for the external id to overwrite the inner one, potentially causing the component's original behavior to fail.

If you want to use a global property chances are you already know it's global.

I bet a fair share, if not most of the devs using id and class don't even know what global attributes are.

This to me is as confusing as an id example, since ARIA is a very specific knowledge and I suspect there are much less people who know about ARIA rather than about Global attributes when working with HTML.

ARIA/a11y is an important but often overlooked aspect of web development, which is why Vue as a framework should provide APIs that facilitate easier a11y best practices. If the whitelist can lead more devs to learn about a11y then it's a good thing.

In the same sense any new built-in HTML element is a breaking change for Vue, but that doesn't prevent us from using those in templates.

That's true, but doesn't mean we have to introduce more of such cases when we can avoid it.


For the suggestion of additional attributes, the reason for not including them is because they are mostly used on content, not components. If you have control over the components you are authoring, then you can always just add them directly in your own components. Fallthrough is typically used on 3rd party components you don't have control over, and these 3rd party components are almost always reusable, utility components. If they are used to display content, they will accept content via slots - which is controlled by you.

The only exception is probably tabindex, which we should consider adding.


I can sense most of your objections root from the idea that a whitelist may lead to confusion when things don't work because it's not in the whitelist, but as we agree that the whole global attributes list doesn't make sense, then I think it's best to keep the whitelist minimal for only ones with high probability of being needed for fallthrough.

@Justineo
Copy link
Member

Justineo commented Mar 1, 2020

I want to mention that ids are required to work with some aria-* attributes like aria-labelledby. Maybe we should make it work the same as other A11Y attributes.

@CyberAP
Copy link
Contributor

CyberAP commented Mar 1, 2020

There's also a hidden attribute left out of the discussion. It's a soft style="display: none" and works about the same as v-show but has lower specificity. It has to be considered as well I think.

@leopiccionia
Copy link

leopiccionia commented Mar 1, 2020

I'm currently divided about tabindex fallthrough, specifically.

While it's a global attribute, tabindex is not accessibly applied in non-interactive elements (a condition defined by their ARIA roles). So, perhaps, applying tabindex to the right element should be handled more carefully by the component's architecture.


By the way, how are conflicts between explicit attributes in the root handled? I suppose class lists are simply merged, and the same algorithm would work with explicit v-bind="$attrs".

Given a component Comp whose template is:

<template>
  <div role="button" style="text-decoration: underline; color: green">
    <slot/>
  </div>
</template>

If it's consumed like this:

<Comp role="link" style="color: red">
  Content
</Comp>

Should it render like option 1 or 2 below?

<!--- Option 1: Final word by component provider -->
<div role="button" style="text-decoration: underline; color: green">
  Content
</div>

<!-- Option 2: Final word by component consumer -->
<div role="link" style="text-decoration: underline; color: red">
  Content
</div>

I'm more inclined to option 1 (final word by component provider).

@tmorehouse
Copy link

What about data-* attributes? how are they handled with the fall-through?

@tmorehouse
Copy link

I'm also not sure about how to deal with listeners when they are part of this.$attrs now, unless there will be a way to declare custom events when defining a component (conflicts between custom events and native events such as input, change), and how users would handle listening for the native event instead of the custom event (i.e. with .native modifier now removed)?

@posva
Copy link
Member

posva commented Mar 1, 2020

@tmorehouse
Copy link

tmorehouse commented Mar 1, 2020

@posva re custom events, there still can be confusion between custom component events and native events that use the same name (i.e. input, change, etc)

@JamesCoyle
Copy link

Why is $listeners removed? It may be useful in some situations to handle events listeners differently to attributes when manually applying them. I don't see why they can't still be kept separate even if they will be treated as native events by default. You could still spread them onto child components alongside the attributes using v-bind="{...$attrs, ...$listeners}".

How does this work with multiple root nodes? Do the passed attributes apply to all root nodes or will the fallthrough be disabled for such components?

I like having to define props and would like the proposal for emits too so it is absolutely clear exactly what the component handles and what it doesn't. I don't think defining props should be optional. I'd rather just have everything that isn't defined in props or emits passed to the DOM as native attributes by default.

Why does it need to be made complicated and confusing by only having a few attributes pass through? Everything should be inherited or nothing should be inherited otherwise it's confusing as to why some things work and others don't.

The users of a component should be able to add whatever attributes they want without having to worry about the component author passing them through to the child components. Components should only have control over attributes/events they have explicitly defined in props/emits IMO unless they set inheritAttrs to false to gain full control over all attributes and events. I'd even suggest adding a separate inheritEvents option for even finer control.


inheritAttrs: false does not affect class and style.

So make it affect those attributes. If the component author specifies inheritAttrs: false they should be responsible for passing these attributes to the child elements if required.

Implicit fallthrough does not apply for event listeners, leading to the need for .native modifier if the user wish to add a native event listener to the child component root.

Have event listeners inherited by default so we can get rid of .native but keep them on $listeners if inheritAttrs is false to allow more flexibility to component authors.

class, style and v-on listeners are not included in $attrs, making it cumbersome for a higher-order component (HOC) to properly pass everything down to a nested child component.

Treat class and style like any other attribute and treat v-on listeners as they currently are. $attrs and $listeners can easily be merged together by the component author if required but they are harder to separate if already merged.


In short, I feel like components should have to explicitly define what they can control and everything else should be passed down to the child components. Component authors should be given more control to declare exactly what their component needs to access.

@yyx990803
Copy link
Member Author

Closing - superseded by #154

@yyx990803 yyx990803 closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants