Ember.js: onEventName= vs. oneventname= regression in Ember 3.1-beta

[mwpastore]

The following template code works in 3.0 (and as far as I know Ember 1.x and 2.x) but not in 3.1-beta:

<button onClick={{action 'foo'}}>

Down-casing the attribute to onclick "solves" the problem.

Developers coming from other frameworks or vanillajs probably expect this to work. And whether it should or shouldn't work, it will probably break some folks upgrading to Ember >3.0.

[rwjblue]

Thanks for reporting! I'm not 100% sure if this will end up being considered a regression or not, I'll try to discuss in more detail with the team and decide...

[tomdale]

Per discussion at the core team meeting, we want to treat this as a regression. Off the top of my head I'm not aware of any changes in the VM that should have caused this, so will need some investigation.

[st-h]

is there anything one can do to help move this forward? This regression is keeping us from upgrading as there is no workaround for the lowercase viewbox which breaks rendering of svgs. But we really do want to upgrade as we really would like to finally get rid of the wrapper div, which is regularly inconveniencing us :cyclone:

[rwjblue]

@st-h - TBH, we mostly need someone to dig and help track down why we regressed. Perhaps a great first step is submitting a failing test case PR both here and in glimmer-vm...

[st-h]

TLDR: this message is most likely not related to this issue

@rwjblue reporting back from my first extensive debugging adventure through the glimmer and ember internals :neckbeard:
I tried to create a test first, however my efforts to write something that matches the current implementations only showed that the current test setup happily accepts any diffs in case sensitive bindings (both in glimmer.vm and ember.js). Probably the test helpers should be refined... However, there are too many internals involved and I don't know the codebase well enough to make a substantial decision here.
So, I continued to find out what is actually going wrong and I believe to have found the offending commit / code. This essentially changed using the lower case name of the attribute when setting it within the install method
Yet again, being very new to the ember/glimmer internals, I don't feel comfortable deciding what is the best way to fix this. I am happy to help getting this done with a little support though.

[rwjblue]

Hmm. That change definitely does odd. Do Ember's own tests pass if you remove that .toLowerCase()? I'm unsure how this could be the cause of <button onClick=(action 'foo')></button)> not working because it seems like it would only be related to component's attribute bindings right?

[st-h]

@rwjblue maybe I should have posted that to the svg related issue, as I am not totally sure if both issues really do have the same cause. However, I was specifically looking why an attributeBinding like viewBox would end up as viewbox.
If I do make requested change there is one test which fails, which specifically tests for case insensitivity. Honestly this seems a little odd to me, since I am not used to js being case insensitive.

[st-h]

I was able to write a test within ember.js and not glimmer.vm so far. Is there anything I can look at for some inspiration?

This is what I have done for ember.js within event-dispatcher-test.js:

    ['@test case insensitive event handler'](assert) {
      let receivedEvent;

      this.registerComponent('x-bar', {
        ComponentClass: Component.extend({
          clicked(event) {
            receivedEvent = event;
          },
        }),
        template: `<button id="is-done" onclick={{action clicked}}>my button</button>`,
      });

      this.render(`{{x-bar}}`);

      this.runTask(() => this.$('#is-done').trigger('click'));
      assert.ok(receivedEvent, 'change event was triggered');
      assert.strictEqual(receivedEvent.target, this.$('#is-done')[0]);
    }

    ['@test case sensitive event handler'](assert) {
      let receivedEvent;

      this.registerComponent('x-bar', {
        ComponentClass: Component.extend({
          clicked(event) {
            receivedEvent = event;
          },
        }),
        template: `<button id="is-done" onClick={{action clicked}}>my button</button>`,
      });

      this.render(`{{x-bar}}`);

      this.runTask(() => this.$('#is-done').trigger('click'));
      assert.ok(receivedEvent, 'change event was triggered');
      assert.strictEqual(receivedEvent.target, this.$('#is-done')[0]);
    }

The second test currently fails. However, I am not sure if that is the best place for such a test. Maybe that can even be simplified. Please let me know if I should submit a PR for that, or if it should be done differently.

[st-h]

this seems to be quite a though one, as there have been several non trivial changes in how glimmer vm handles attributes. I tried to debug through some changes and it seems that this issue appears after the DynamicProperty is no longer instantiated with a property manager (as is in 0.25.6) that contains the normalized attribute. By now this exceeds all my knowledge about glimmer, its intention and development. Hopefully this helps in resolving this issue though. However by now I am pretty sure that other than what we expected, this issue is not related to #16477

[rwjblue]

@st-h - Those tests look great, a failing test PR would be perfect! 👍

[rwjblue]

FYI - The fix for this has been pulled into release (for 3.1.x release) and beta (for 3.2.0-beta.x release). The latest builds (in 10-15 minutes) to the release, beta, and canary channels should include the fix (you can get the latest tarball URL via ember-source-channel-url).