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.

Sign up for GitHub

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
Merged

[BUGFIX beta] attributeBindings not working with role #14009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions packages/ember-glimmer/tests/integration/components/attribute-bindings-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,4 +517,26 @@ moduleFor('Attribute bindings integration', class extends RenderingTest {

this.assertComponentElement(this.firstChild, { tagName: 'a', attrs: { href: 'unsafe:javascript:alert(\'foo\')' } });
}

['@test it can bind the role attribute (issue #14007)']() {
let FooBarComponent = Component.extend({ attributeBindings: ['role'] });

this.registerComponent('foo-bar', { ComponentClass: FooBarComponent, template: 'hello' });

this.render('{{foo-bar role=role}}', { role: 'button' });

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { role: 'button' } });

this.runTask(() => this.rerender());

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { role: 'button' } });

this.runTask(() => set(this.context, 'role', 'combobox'));

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { role: 'combobox' } });

this.runTask(() => set(this.context, 'role', null));

this.assertComponentElement(this.firstChild, { tagName: 'div' });
}
});
2 changes: 1 addition & 1 deletion packages/ember-htmlbars/lib/system/build-component-template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.


if (attrs.tagName) {
component.tagName = attrs.tagName;
Expand Down