Angle bracket component with splattributes not correctly merging attributes

[barryofguilder]

In my Ember 3.10 app, I have a button component that has tageName: '' set, and then in the template, I'm using splattributes. I'm getting some unexpected behavior when I try to override the attributes.

Below are some code snippets to explain it, but you can see it in detail in my repository here.

Here is my button component:

import Component from '@ember/component';
	
export default Component.extend({
  tagName: ''
});

and template

<button type="button" data-test-id="button" ...attributes>
  {{yield}}
</button>

If I use my button component by itself, the data-test-id attribute is overriden correctly, but the type attribute is still using the component's default of "button":

<XButton type="submit" data-test-id="submit-btn">Save</XButton >

{{!-- generated HTML --}}
<button type="button" data-test-id="submit-btn">Save</button>

If I'm yielding it out with the component template helper, it does not correctly override either of them:

<form>
  {{yield
    (
      hash
        submit=(
          component
            "x-button"
            data-test-id="submit-btn"
            type="submit"
        )
    )
  }}
</form>

{{!-- generated HTML --}}
<form>
  <button type="button" data-test-id="button">Save</button>
</form>

This doesn't seem correct to me. It seems that at least in the scenario where I'm using the component directly, it should override all of the attributes. The component helper scenario might make sense if it's only passing component properties and not element attributes.

[chancancode]

I think this was fixed in 3.11. Mind upgrading?

[barryofguilder]

The repo that I linked to is actually using 3.11.1. 😬

[chancancode]

Hmm, turns out this is indeed still a bug. This is specific to the type attribute. This patch changed things to ensure that type attribute is set last, but did not take into account ...attributes overwriting. I think the compile-time fix in the original patch is probably not viable, and it needs to be reverted in favor of a runtime patch using the Operations API (for component elements at least).

[barryofguilder]

What about the use case with the component template helper? In that one, neither is working. Will that not work with angle bracket components and splattributes?

[chancancode]

What about the use case with the component template helper?

Sorry.. I'm no sure if I understand what you mean here

[barryofguilder]

The second part of my original post where I have this code:

<form>
  {{yield
    (
      hash
        submit=(
          component
            "x-button"
            data-test-id="submit-btn"
            type="submit"
        )
    )
  }}
</form>

{{!-- generated HTML --}}
<form>
  <button type="button" data-test-id="button">Save</button>
</form>

It seems that the component template helper isn't correctly merging any of the attributes.

[chancancode]

There is no way to pass html attributes to the component helper at all, but that is a different issue/feature reauest

[barryofguilder]

Ok, I will make a new issue for that use case.

[rwjblue]

This is specific to the type attribute. This patch changed things to ensure that type attribute is set last, but did not take into account ...attributes overwriting.

Yeah, ...attributes was not a thing when that commit landed. This also explains another open issue (RE: <input>s type attribute not being overridable with ...attributes).

[robclancy]

This is kind of a big bug? You can't use the type attribute at all with glimmer components and need to use an argument.

The fact you can't do anything with attributes passed in makes this worse, it is basically impossible to use type="submit". The fact you can't access ...attributes separately or on the js side is annoying already. Now I have an attribute I can not use at all and have to remember to do @type="submit" everytime.

[pzuraq]

Can we potentially update the logic such that the merging happens before we assign any values at all? That way we can still assign type last, it'll just be the final, merged value of type.

[gitKrystan]

We ran into this issue on our app too. Here is a twiddle repro.

[sukima]

How do we upvote bugs? I have a feeling this one is going to cause all kinds of confusion pretty quickly.

[locks]

@sukima there is no upvoting mechanism. The solution to this bug is also not completely straightforward, as changing the timings of the merging might impact other use cases and attributes.

[lougreenwood]

Does this bug need tracking in the https://github.com/glimmerjs/glimmer-vm repo? I don't see a linked issue, but I guess this is a Glimmer VM problem since there was reference to some Glimmer commit above?

[Techn1x]

Also hit this bug today, very confusing. If we aren't able to solve it quickly, maybe the docs could be updated to mention this button type exception and any potential workarounds
https://guides.emberjs.com/release/components/component-arguments-and-html-attributes/#toc_html-attributes

[alexlafroscia]

We ran into this one today, too, but with a component wrapping an input field and wanting to be able to override the type attribute

https://ember-twiddle.com/04370035f58cbe203bb18c3df548f62e?openFiles=templates.components.text-input%5C.hbs%2C

[rreckonerr]

@rwjblue @chancancode Hi! Any updates on this one? Still see this issue on 3.20.0

[robclancy]

This has come up on discord again. Not documented. No error. Just lucky if someone manages to find this thread or asks on discord and someone is around that knows the issue.
This is a major bug open for over a year.

[pzuraq]

I believe this fix should fix this issue: glimmerjs/glimmer-vm#1178

The PR is approved and everything, unsure why it didn't get merged. I'll review this week if no one else takes a look by then.

[jelhan]

@pzuraq Shouldn't this be open until Glimmer VM is updated in Ember itself?

[pzuraq]

@jelhan it was autoclosed since the linked issue was merged, didn't notice that until now (thanks for reopening @rwjblue!). The hazards of cross-repo dependencies 😩

However that fix now has been merged in #19293

So I think we can officially close this! Please reopen if you still see this issue on Canary

[DLiblik]

FYI - in case anyone is dealing with this currently - we had a nasty surprise when the type attribute issue resulted in an enterprise app not applying the "password" value properly, resulting in a slew of support issues as people had already typed their pw in before looking up and realizing it was on their screen in plaintext because the field became a normal text input. The other issue was that their pw auto-fill was not working (also because it won't if the type="password" attribute is missing).

This gave us a bit of a black eye as it caused a secondary wave of security concerns around using open-source platforms like Ember for mission-critical apps. We got everyone tucked back into bed, but this one was pretty challenging as it sailed through automated testing (since the login form tests didn't check if the input had type=pw, just that the form worked - we plugged that hole!).

Good learning for us, but this issue probably deserves a call-out in all prior ember versions where it may have impact.

[robclancy]

Yep, this should never have been an issue in the first place and then ignored for 6 months after a casual acknowledgement that it was a real bug. Then ignored for almost a whole year before being merged (after it once again being ignored for the majority of the year until I commented again because someone on discord had this issue yet again).

What this issue represents is bigger than the issue itself. There is so much mess left in Ember to stay backwards compatible but really it doesn't.

[DLiblik]

@robclancy I was raising our experience only to indicate that there is at least one scenario (the one I identified) that has implications beyond a purely functional issue - i.e. security - and so that may raise a flag to folks on the docs side to splat something in somewhere to call this issue out on earlier versions.

I do not find Ember anything less than extraordinary as a platform - yes it's imperfect, but technology itself is by definition the risk of choosing change and innovation over the status quo, and I find the balance of risks to be handled very well here. I wrote my first line of code in 1978 and have written over 2M since across some 50+ platforms, languages, and OS's, from assembly up to 4GLs and this is one of the best run I've seen, esp. given its budget!

[pzuraq]

@robclancy FWIW, there were a number of procedural failures that led to the long delay for solving this bug as well, primarily:

1. The bug existed in the Glimmer VM
2. The Glimmer VM had drifted significantly from Ember by the time this bug was reported, it was several major refactors deep into major changes
3. Said refactors were buggy, as they had not been integrated in the meantime, and also caused a performance regression when integrated.

This made the upgrade process very difficult. We did patch some changes to previous versions of the VM when necessary, but we tried to avoid it as much as possible because it was causing even more divergence, which was making it even more difficult to catch up.

After finally landing the VM upgrade in 3.17, we spent several months fixing the ensuing regressions, and then several more months refactoring the VM itself, along with our processes surrounding VM changes and upgrades. We now have a policy of always integrating immediately when we make breaking changes, along with automated performance benchmarks to make sure we don't regress in the VM. A large portion of the internals of Ember have also been moved up into the VM, reducing duplication and complexity overall. The goal here is to make sure we never get into that type of situation again, and that bugfixes like these should generally be easy to implement and fix.

We also were not aware that this bug could represent a security concern. That would have bumped it up the priority list significantly - we're always fixing bugs and issues, and this one was concerning but not the highest priority because it had workarounds and did not appear to be impacting many people compared to many other bugs (the bugs from the VM upgrades were breaking people's apps regularly). Thank you @DLiblik for bringing this to our attention!

[sukima]

I do not find Ember anything less than extraordinary as a platform

I so wish this sentimentality could extend beyond the walled garden and into the general consciousness of the industry at large.

[andreyfel]

This issue is fixed in 3.25.*. For some reason, the changelog doesn't mention it https://github.com/emberjs/ember.js/blob/master/CHANGELOG.md.