ember.js: Angle bracket component with splattributes not correctly merging attributes

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.

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 9
Comments: 28 (15 by maintainers)

Commits related to this issue

Un-deprecate BsButton's buttonType argument Due to https://github.com/emberjs/ember.js/issues/18232 we still support the attribute-argument in current 4.0 RCs, so remove the deprecation here to not c... — committed to ember-bootstrap/ember-bootstrap by simonihmig 4 years ago
Un-deprecate BsButton's buttonType argument Due to https://github.com/emberjs/ember.js/issues/18232 we still support the attribute-argument in current 4.0 RCs, so remove the deprecation here to not c... — committed to ember-bootstrap/ember-bootstrap by simonihmig 4 years ago

Most upvoted comments

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.

robclancy on Jan 27, 2020

@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!

DLiblik on Jan 3, 2021

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.

robclancy on Nov 27, 2020

I believe this fix should fix this issue: https://github.com/glimmerjs/glimmer-vm/pull/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.

pzuraq on Nov 28, 2020

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.

sukima on Jan 4, 2021

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

The bug existed in the Glimmer VM
The Glimmer VM had drifted significantly from Ember by the time this bug was reported, it was several major refactors deep into major changes
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!

pzuraq on Jan 4, 2021

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

sukima on Apr 10, 2020

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.

andreyfel on Mar 22, 2021

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.

DLiblik on Jan 3, 2021