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

fix(checkbox): added aria-checked=mixed to indeterminate state #2389

Merged
merged 4 commits into from
Mar 13, 2018

Conversation

aprigogin
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #2389 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2389      +/-   ##
==========================================
+ Coverage   98.85%   98.86%   +<.01%     
==========================================
  Files         100      100              
  Lines        4118     4123       +5     
  Branches      527      528       +1     
==========================================
+ Hits         4071     4076       +5     
  Misses         47       47
Impacted Files Coverage Δ
packages/mdc-checkbox/constants.js 100% <ø> (ø) ⬆️
packages/mdc-checkbox/foundation.js 97.58% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f59b109...5bfba7f. Read the comment docs.

Copy link
Contributor

@patrickrodee patrickrodee left a comment

Choose a reason for hiding this comment

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

Got a question about potential race conditions

@@ -203,6 +211,10 @@ class MDCCheckboxFoundation extends MDCFoundation {
return;
}

// Remove aria-checked (needed for initial inditerminate state) - screen
// readers will pick the right state from input.
this.adapter_.removeNativeControlAttr(strings.ARIA_CHECKED_ATTR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a possible race condition between when the property change callback gets called and when the attribute gets added?

If so, the attr settter could be moved inside this (transitionCheckState_) method with a check made to newState to determine whether to add or remove.

@patrickrodee patrickrodee self-assigned this Mar 13, 2018
Copy link
Contributor

@patrickrodee patrickrodee left a comment

Choose a reason for hiding this comment

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

Make sure to add the breaking change notes to the footer of the commit message: https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#footer

LGTM!

@aprigogin aprigogin merged commit 416512f into master Mar 13, 2018
kfranqueiro pushed a commit that referenced this pull request Mar 13, 2018
BREAKING CHANGE: Adds setNativeControlAttr and removeNativeControlAttr adapter APIs.
@kfranqueiro kfranqueiro deleted the fix/checkbox/a11y branch August 1, 2018 16:22
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