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(commons/dom): change isFocusable to check visibility in screenreader mode #655

Closed

Conversation

0ddfell0w
Copy link
Contributor

Lets isFocusable tell the difference between elements that are still focusable despite being invisible

fixes #647

Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

I would like to see some additional tests:

  1. display:none is on an ancestor
  2. ancestor is clipped
  3. ancestor is off screened
  4. visibility:hidden on ancestor

@0ddfell0w 0ddfell0w force-pushed the focusable-screenreader-visible branch from ab16de5 to 1610a82 Compare December 15, 2017 19:30
@0ddfell0w
Copy link
Contributor Author

@dylanb done

@dylanb
Copy link
Contributor

dylanb commented Dec 15, 2017

Leaving for @WilcoFiers to determine when to merge

@0ddfell0w
Copy link
Contributor Author

0ddfell0w commented Dec 18, 2017

@dylanb i actually think this might not work, isFocusable() will return True for the unfocusable <button style="visibility:hidden" tabindex="0">Hidden button still not focusable</button>

isFocusable will have the following logic:
is it natively focusable? No, because it's hidden
is it a button: Yes, therefor it's focusable.

@0ddfell0w
Copy link
Contributor Author

I'll add tests to isFocusable for the disabled/screenreader-invisible cases as part of resolving this issue

@0ddfell0w
Copy link
Contributor Author

@dylanb @WilcoFiers didn't want to force push over approved changes, so here's another PR with the changes I think are correct: #658

@WilcoFiers
Copy link
Contributor

@0ddfell0w Does that mean we can close this? If so, please go ahead.

@0ddfell0w 0ddfell0w closed this Dec 19, 2017
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.

Bug: dom.isFocusable mistakenly concludes that invisible elements are unfocusable
3 participants