Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[fixed] some untabbable elements are being returned from findTabbable #774

Merged
merged 3 commits into from
Oct 4, 2019

Conversation

flurmbo
Copy link
Contributor

@flurmbo flurmbo commented Oct 3, 2019

Fixes #732

Changes proposed:

The bug in the example stems from the hidesContents function in tabbable.js returning false on the button with the display: none inline style. It is returning false because the element has the computed style overflow: visible (which is default value).

The problem with this is that having overflow: visible doesn't mean there is any content that is overflowing.

I think there are a couple possible ways to fix this. One would be to add an additional check for the display: none style if zeroSize === true, which would directly solve the bug in the example.

I'm not sure that will catch every possibility, so instead I check the elements scrollWidth and scrollHeight to determine whether or not there is any overflowing content.

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • If this is a code change, a spec testing the functionality has been added.

@coveralls
Copy link

coveralls commented Oct 3, 2019

Coverage Status

Coverage remained the same at 86.682% when pulling 6b1cea8 on flurmbo:flurmbo-oct-3 into 4dd25ac on reactjs:master.

@diasbruno
Copy link
Collaborator

@flurmbo This is going to be a hard one, but can you try to add a test case for this?

@flurmbo
Copy link
Contributor Author

flurmbo commented Oct 3, 2019

Hi @diasbruno, sure, should the test just test the tabbable helper function or should it recreate the modal in the example and test that focus stays within the modal?

@diasbruno
Copy link
Collaborator

A test case for tabbable would be great.

@flurmbo
Copy link
Contributor Author

flurmbo commented Oct 4, 2019

Hi @diasbruno, I added a test case for the tabbable helper. It also looks like the previous test was editing the wrong DOM elements style (elem instead of input) so I fixed that in a separate commit.

@diasbruno
Copy link
Collaborator

Thanks, @flurmbo. I'll merge this and make a release tomorrow.

@diasbruno diasbruno merged commit 9be00a9 into reactjs:master Oct 4, 2019
programmer4web pushed a commit to programmer4web/react-modal that referenced this pull request Oct 5, 2020
…reactjs#774)

* [fixed] some untabbable elements are being returned from findTabbable

- add test
- fix apparent typo in test spec
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus escapes modal when first or last element has style display: none
3 participants