Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nbibler opened this issue Mar 16, 2018 · 3 comments
Closed
Assignees
Milestone

Comments

@nbibler
Copy link
Contributor

nbibler commented Mar 16, 2018

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.

@nbibler nbibler changed the title Unexpected behavior change of ariaRole with glimmer-vm upgrade Unexpected behavior change of ariaRole with v3.1.0-beta.1 glimmer-vm upgrade Mar 16, 2018
nbibler added a commit to IvyApp/ivy-tabs that referenced this issue Mar 16, 2018
@rwjblue rwjblue added the Bug label Apr 21, 2018
@rwjblue rwjblue added this to the v3.1.x milestone Apr 21, 2018
@rwjblue
Copy link
Member

rwjblue commented Apr 21, 2018

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
Copy link
Member

rwjblue commented Apr 21, 2018

Should be addressed by #16563.

@rwjblue
Copy link
Member

rwjblue commented Apr 21, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants