Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngAria): don't attach roles to native controls #14145

Closed
wants to merge 1 commit into from
Closed

fix(ngAria): don't attach roles to native controls #14145

wants to merge 1 commit into from

Conversation

m-amr
Copy link
Contributor

@m-amr m-amr commented Feb 27, 2016

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    fix
  • What is the current behavior? (You can also link to an open issue here)

attach roles to these native element (textarea, select, button)

  • What is the new behavior (if this is a feature change)?

don't attach roles to these native these element (textarea, select, button)

  • Does this PR introduce a breaking change?

no

don't add roles to (textarea, button, select, input)
fix #14076

@m-amr
Copy link
Contributor Author

m-amr commented Feb 27, 2016

I can't found anything to be updated in the documentation.

@m-amr m-amr force-pushed the roles_for_native_elements branch 4 times, most recently from fc6e726 to b8f8822 Compare February 28, 2016 00:09
gkalpak
gkalpak reviewed Feb 28, 2016
View reviewed changes
test/ngAria/ariaSpec.js
compileElement('<a ng-click="doClick()"></a>');
expect(element.attr('role')).toBe(undefined);
});

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Redundant newline

@gkalpak
Copy link
Member

gkalpak commented Feb 28, 2016

Not super important, but it's more concise to use the they helper for tests (since they are pretty repetitive).
Could you also update the commit message to mention this is a BC (albeit a small one that shouldn't affect a11y) ?

@gkalpak gkalpak added this to the 1.6.x milestone Feb 28, 2016
@m-amr m-amr force-pushed the roles_for_native_elements branch from b8f8822 to fed351a Compare February 28, 2016 20:01
@m-amr
Copy link
Contributor Author

m-amr commented Feb 28, 2016

Thanks for your feedback, I have updated the code
1 - I am now using 'they' instead of 'it'
2 - I have updated the commit message.

Thanks.

@Narretz
Copy link
Contributor

Narretz commented Mar 30, 2016

The commit message must still be updated to conform with our standard on Breaking changes: https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit#heading=h.gbbngquhe0qa

@m-amr m-amr force-pushed the roles_for_native_elements branch 3 times, most recently from 0eb5441 to 2f981b7 Compare March 30, 2016 17:33
@m-amr
Copy link
Contributor Author

m-amr commented Mar 30, 2016

@Narretz
I have update the commit message to confirm the standard of the breaking change.
please check it.
Thanks

@Narretz
Copy link
Contributor

Narretz commented Mar 30, 2016

@m-amr you still need to add an actual explanation of the breaking change, because that will show up in the changelog, e.g. roles are not added to native input / form elements anymore, and that this should not affect accessibility, because native inputs are accessible by default.

@m-amr m-amr force-pushed the roles_for_native_elements branch 2 times, most recently from 2d2af6b to e101e8d Compare March 30, 2016 21:15
@m-amr
Copy link
Contributor Author

m-amr commented Mar 30, 2016

@Narretz i have added some explanation about the change
could you please check it again ?

@m-amr m-amr force-pushed the roles_for_native_elements branch from e101e8d to 185b982 Compare March 30, 2016 22:34
@Narretz Narretz self-assigned this Apr 7, 2016
@m-amr m-amr force-pushed the roles_for_native_elements branch from 185b982 to 6fd9c76 Compare April 8, 2016 03:40
@m-amr
fix(ngAria): don't add roles to native elements
6fd9c76 
prevent ngAria from attaching roles to textarea, button, select, summary, details, a and input

Closes  angular#14076

BREAKING CHANGE:

Curreny behavior is that ngAria don't attach roles to input elements but it does so to other native controls.
so this change will prevent ngAria from attaching roles to native elements like (textrea, select, button, etc) and it will only attach roles to non-native controls.
and this should not affect accessibility, because native inputs are accessible by default.
(albeit a small one that shouldn't affect a11y)
@Narretz Narretz closed this in 9978de1 Apr 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@Narretz Narretz

Labels
cla: yes component: ngAria frequency: moderate needs: breaking change severity: broken expected use type: bug
Projects
None yet
Milestone
1.6.x
Development

Successfully merging this pull request may close these issues.

fix(ngAria): don't attach roles to native controls
4 participants
@m-amr @gkalpak @Narretz @googlebot