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

tests(a11y): use regex for target size explanation #15231

Merged
merged 2 commits into from Jul 7, 2023
Merged

Conversation

adamraine
Copy link
Member

The exact target sizes vary depending on the device. For example, the failing target was 8px by 18px on my macbook.

@adamraine adamraine requested a review from a team as a code owner July 7, 2023 18:01
@adamraine adamraine requested review from connorjclark and removed request for a team July 7, 2023 18:01
@@ -859,7 +859,8 @@ const expectations = {
'type': 'node',
'selector': 'body > section > span#target-size-2',
'snippet': '<span role="button" tabindex="0" id="target-size-2">',
'explanation': 'Fix any of the following:\n Target has insufficient size (8px by 17px, should be at least 24px by 24px)\n Target has insufficient offset from its closest neighbor (12px should be at least 24px)',
// Exact target size can vary depending on the device.
'explanation': /^Fix any of the following:\n {2}Target has insufficient size \([0-9]+px by [0-9]+px, should be at least 24px by 24px\)\n {2}Target has insufficient offset from its closest neighbor \([0-9]+px should be at least 24px\)$/,
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not /s+?

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal was to match the exact string as much as possible. \s+ could be any number of any whitespace whereas this only matches exactly two spaces.

Doesn't matter to me much but this is what eslint recommended when I copied the string into the regex.

@connorjclark
Copy link
Collaborator

This is the sort of thing we expect our emulation settings (viewport and DPI) to control for, isn't it?

@adamraine
Copy link
Member Author

It may just be outside the control of our emulation settings. From what I can tell it could be the way fonts are rendered on different devices. The difference in size is never more than 1-2px.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

We should merge this but I think we ought to add a test backlog issue about it. I feel like we're putting a bandaid on an emulation bug.

@adamraine adamraine merged commit c2ab38a into main Jul 7, 2023
33 checks passed
@adamraine adamraine deleted the a11y-target-regex branch July 7, 2023 20:30
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.

None yet

2 participants