emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.47k stars 4.21k forks source link

Unexpected behavior change of ariaRole with v3.1.0-beta.1 glimmer-vm upgrade #16379

Closed nbibler closed 6 years ago

nbibler commented 6 years ago

PR #15828 seemed to introduce a (unexpected?) change to the behavior of ariaRole on Components.

In previous versions of Ember, the ariaRole is allowed to be dynamic and the role attribute on the generated element would update as expected. If it were initially falsy, there would be no role attribute. If it later became truthy, a role attribute would be appended.

Once the PR was merged into v3.1.0-beta.1 (at 36ac96a), there is now a conditional check which optionally binds the attribute. Only if ariaRole is truthy at instantiation will it be monitored for future changes. If it's initially falsy, no binding will be setup and any future changes to ariaRole will not add a role attribute to the element.

The change is highlighted below:

--- a/packages/ember-glimmer/lib/component-managers/curly.ts
+++ b/packages/ember-glimmer/lib/component-managers/curly.ts
@@ -414,6 +413,12 @@ export default class CurlyComponentManager extends AbstractManager<ComponentStat
         ClassNameBinding.install(element, component, binding, operations);
       });
     }
+    operations.setAttribute('class', PrimitiveReference.create('ember-view'), false, null);
+
+    const ariaRole = get(component, 'ariaRole');
+    if (ariaRole) {
+      operations.setAttribute('role', PrimitiveReference.create(ariaRole), false, null);
+    }

     component._transitionTo('hasElement');

Was this an intended behavior change? If so, I don't see it called out in the CHANGELOG.

rwjblue commented 6 years ago

Oh no! I'm sorry I missed this issue! The basic change needed here is to do ariaRole in component instead of if (get(component, ariaRole)) {} like was done above. It will still do less work than before (most components do not have or use ariaRole at all), but it will allow a component to not set an ariaRole by returning a falsey value.

rwjblue commented 6 years ago

Should be addressed by https://github.com/emberjs/ember.js/pull/16563.

rwjblue commented 6 years ago

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).