-
Notifications
You must be signed in to change notification settings - Fork 808
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
[fixed] element with display 'contents' is visible and is tabbable. #906
[fixed] element with display 'contents' is visible and is tabbable. #906
Conversation
@aloker Thanks for the patch...can you have a look on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few comments
function hasElementOverflow(element, style) { | ||
return style.getPropertyValue("overflow") !== "visible" || | ||
// if 'overflow: visible' set, check if there is actually any overflow | ||
(element.scrollWidth <= 0 && element.scrollHeight <= 0); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the naming backwards? If the function returns true, the element does not overflow (either because its content is clipped or its content is actually 0x0 in size)
const displayValue = style.getPropertyValue("display"); | ||
return displayValue !== DISPLAY_CONTENTS && (zeroSize | ||
? hasElementOverflow(element, style) | ||
: displayValue === DISPLAY_NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd go with
return zeroSize
? displayValue !== DISPLAY_CONTENTS && hasElementOverflow(element, style)
: displayValue === DISPLAY_NONE;
but that's mostly a matter of taste. Either way works.
Nice. I'll just add a test and we are good to go... |
@diasbruno how can we help to get this PR out. We use the |
@galexandrade That's great! I believe the only thing missing here is to add a tests. You can take this PR and improve it. |
@diasbruno @aloker I checked out the branch and tried to add some tests to cover the scenario but since the problem only happens on Safari, it is hard to simulate the same scenario on ReactDOM. This is the problem I would like to have fixed here: Screen.Recording.2022-10-12.at.3.06.32.PM.movYou can check this example in Safari and Chrome. That works fine, except for Safari where it won't tab through the elements. With this PR, the issue is fixed. I'd like to offer help to move this PR forward. Please, how can I help to get this PR merged? 🙏 |
@galexandrade There are a few comments on the PR. You can take this patch and improve on it. |
Fixes #905.
Changes proposed:
Upgrade Path (for changed or removed APIs):
Acceptance Checklist:
CONTRIBUTING.md
.