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

feat(frame-focusable-content): new rule to test iframes with tabindex=-1 do not have focusable content #2785

Merged
merged 9 commits into from
Feb 2, 2021

Conversation

straker
Copy link
Contributor

@straker straker commented Jan 28, 2021

Pulling out more of #2762 to see if it fixes the ie11 problem that only happens in circle.

Note: The after method is incomplete on purpose. This is just the minimalistic amount of code necessary in order to get the rule to run.

@straker straker requested a review from a team as a code owner January 28, 2021 22:31
@straker
Copy link
Contributor Author

straker commented Jan 29, 2021

For whatever reason the thing that cases frame-test rule to fail is adding a nested iframe to this rules integration test. It doesn't matter if the after method is empty or not. I have no idea why, so we are going to just get this through for now and add a tech debt ticket to investigate it.

let width = parseInt(frame.node.getAttribute('width'), 10);
let height = parseInt(frame.node.getAttribute('height'), 10);
width = isNaN(width) ? rect.width : width;
height = isNaN(height) ? rect.height : height;
Copy link
Contributor Author

@straker straker Jan 29, 2021

Choose a reason for hiding this comment

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

This prevents margin/border/padding CSS from affecting the width/height (otherwise in Chrome a width and height of 1 would result in a rect size of 5x5).

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Minor change request

test/rule-matches/frame-focusable-content-matches.js Outdated Show resolved Hide resolved
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.

2 participants