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
Narretz merged 1 commit into
angular:masterfrom
Narretz:fix-aria-invalid
Dec 11, 2017
Merged

fix(ngAria): do not set aria attributes on input[type="hidden"]#16367
Narretz merged 1 commit into
angular:masterfrom
Narretz:fix-aria-invalid

Conversation

@Narretz
Copy link
Copy Markdown
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

@Narretz
fix(ngAria): do not set aria attributes on input[type="hidden"]
bf46753 
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 Narretz added this to the 1.7.0 milestone Dec 8, 2017
@Narretz Narretz requested a review from gkalpak December 8, 2017 12:48
@Narretz Narretz mentioned this pull request Dec 8, 2017
Closed
@Narretz
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@ghost ghost mentioned this pull request May 11, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gkalpak gkalpak gkalpak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes component: forms component: ngAria frequency: low needs: breaking change type: bug

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Narretz @gkalpak @googlebot