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

fix(ngAria): do not set aria attributes on input[type="hidden"] #16367

Merged
merged 1 commit into from
Dec 11, 2017

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Dec 8, 2017

This fixes a error found by @edclements using the Google Accessibility Developer Tools audit.
Input fields of type hidden shouldn't have aria attributes.
https://www.w3.org/TR/html-aria/#allowed-aria-roles-states-and-properties-1

Closes #15113

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)
aria attributes added to input type hidden

What is the new behavior (if this is a feature change)?
Not anymore.

Does this PR introduce a breaking change?
I would say not actually, because the previous behavior didnt follow the spec and I can'T see why anyone would rely on this behavior. It was marked as breaking change in #15113, though

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

Rebased and updated version of #15113

This fixes a error found by @edclements  using the Google Accessibility Developer Tools audit.
Input fields of type hidden shouldn't have aria attributes.
https://www.w3.org/TR/html-aria/#allowed-aria-roles-states-and-properties-1

Closes angular#15113
@Narretz
Copy link
Contributor Author

Narretz commented Dec 11, 2017

Thanks @gkalpak . Just to clarify, are you okay with thi going into 1.6.x too? Because I currently have not added a BC notice.

@gkalpak
Copy link
Member

gkalpak commented Dec 11, 2017

Hm...you are sneaky 😛
Technically, it is a fix, but it can theoretically break a test. Probably safest to go into 1.7, but I'm on the fence 😃
(Maybe @mgol has a stronger opinion - he added the needs: breaking change to the original PR 😁)

@Narretz
Copy link
Contributor Author

Narretz commented Dec 11, 2017

@gkalpak you said it was a BC and that you would add the BC label: 😛 #15113 (comment)

@gkalpak
Copy link
Member

gkalpak commented Dec 11, 2017

But I didn't 😛
I still mostly agree with myself though (always a good sign 😛).
It is theoretically a BC and I would rather treat it as such. (But if you feel otherwise - and @mgol doesn't have a strong opinion in favor of a BC - I can live with getting it into 1.6.x (as I said in my original comment anyway 😛).)

@Narretz
Copy link
Contributor Author

Narretz commented Dec 11, 2017

But I didn't

Because you forgot 😛
I'll add a BC notice - it's such a small change, and I guess screen readers already ignore hidden inputs, so I doubt this really affects anyone as is

@Narretz Narretz merged commit 6d5ef34 into angular:master Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants