Class names aren't merged correctly when ...attributes is specified first

[GavinJoyce]

There seems to be a difference in how attributes are merged depending on the position of ...attributes within an element. The RFC doesn't seem to specify what the behaviour should be, I'm assuming that position shouldn't matter as otherwise it would likely be a source of confusion.

With the following Foo component template:

<div class="inner" ...attributes />
<div ...attributes class="inner" />

And the following invocation:

<Foo class="outer />

The result is:

<div class="inner outer" />
<div class="outer" />

update: After trying to write a failing test in glimmer-vm, it appears that the classes should be merged and the order will be different. The output should be:

<div class="inner outer" />
<div class="outer inner" />

[GavinJoyce]

Failing test: #17693

[GavinJoyce]

I tried to write a similar failing test in glimmer-vm, it appears to work correctly there:

GavinJoyce/glimmer-vm#1

[rwjblue]

The order was actually intended to matter, though we do have a bug in class merging still (#17533 tracks that) which is what I think you are really reporting here.

The idea was that you could use the location of ...attributes to control if a given attribute could or could not be "clobbered" by your caller.

For example, given these two possibilities:

<div data-foo="inner" ...attributes></div>
<div ...attributes data-foo="inner"></div>

With this invocation:

<Foo data-foo="outer" />

We expect the location of ...attributes to affect the final result:

<div data-foo="outer"></div>
<div data-foo="inner"></div>

[rwjblue]

Also, RE: this working correctly in glimmer-vm, thats because the system doing the merging was overhauled in glimmer-vm but we haven't gotten Ember updated to the newer version (there were breaking changes, and its a bit of work to figure out what exactly is needed to do an upgrade).

[GavinJoyce]

👍 thanks for the explanation. It looks like the tests in #17533 are similar so I'll close #17693.

I'll leave this issue open so that it's discoverable for anyone else hitting the bug

[MelSumner]

If it helps, this would also matter for aria-labelledby because you can concatenate a label from several text nodes- see https://www.w3.org/WAI/GL/wiki/Using_aria-labelledby_to_concatenate_a_label_from_several_text_nodes

[rwjblue]

@MelSumner - Good catch! I think we'll need a small RFC to tweak that behavior since the merged RFC explicitly states that class is the only execption:

This means that attributes provided by the caller will override (replace) those added by the component (except for class, which is merged).

[MelSumner]

for attributes coming from ARIA, there are five whose value is an ID reference list (meaning, they can have one or more values):

• aria-controls - It seems like the order could possibly matter in some instances.
• aria-describedby: order definitely matters
• aria-flowto - the order could matter, depending on intent of content.
• aria-labelledby - order definitely matters
• aria-owns Order could matter, depending on design intent (if someone is using this, really we should make them stop and think twice tbh, it's smelly),

[rwjblue]

👍 thank you for researching that! We should totally do an RFC to get those added to the "plz merge, don't clobber" list. During that RFC cycle we should try to see if there are other attributes like this that should be considered...

[chancancode]

This (the bug in the original thread) should now be fixed on canary and beta!