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

[BUGFIX beta] attributeBindings not working with role #14009

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Aug 3, 2016

Closes #14007

@@ -186,7 +186,7 @@ function normalizeComponentAttributes(component, attrs) {
}
}

normalized.role = buildStatement('get', 'ariaRole');
normalized.role = normalized.role || buildStatement('get', 'ariaRole');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the || by moving the original line up above line 160. That way we always set normalized.role and only if you happen to add an attributeBinding for it would it override (on line 185).

Can you test to see if that fixes the failing test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I see that this is what @krisselden suggested in #14007.

Seems good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However it seems a bit confusing that there is two ways of achieving the same thing. ariaRole="foo" or role="foo" + attributeBindings.
I think that it should be only one blessed approach and the other one should be deprecated.

Copy link

@amk221 amk221 Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a failure of the ability to change ariaRole once set in v3 beta 1.
See here, all tests are passing on Travis apart from one
https://travis-ci.org/zestia/ember-select-box/jobs/352537510

Should I be using ariaRole, or role?

Edit Scratch that, I just changed it to use role and now it's passing fine.

@rwjblue
Copy link
Member

rwjblue commented Aug 3, 2016

Just wanted to confirm that this is working properly in 2.7, but not 2.8. If 2.7 were also broken we would need BUGFIX release...

@rwjblue rwjblue merged commit 3d25f98 into emberjs:master Aug 3, 2016
@cibernox
Copy link
Contributor Author

cibernox commented Aug 3, 2016

2.7 is fine, this bug is only in beta and canary

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

Successfully merging this pull request may close these issues.

3 participants