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(target-size): update to match new spacing requirements #4117

Merged
merged 18 commits into from
Aug 8, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 2, 2023

This mostly coverts the target-offset check to use the updated spacing exception from the spec. However, going over this we did notice that we still might be off in determining the center of the bounding rect is. The code currently only looks at the largest target rect but we are thinking we need to take into account the bounding box for all target rects and center on that. Will leave that as a tech debt ticket for after 4.8 so we can get this merged in on time.

Another tech debt ticket that needs to be created is to update the target-size check to use the new getTargetSize function.

Closes: #3891

@straker straker requested a review from a team as a code owner August 2, 2023 20:56
return { x, y: closestY };
}
}
return Math.hypot(pointA.x - pointB.x, pointA.y - pointB.y);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function exists in IE11 (I've verified)

height: baseRect.bottom - baseRect.top,
width: baseRect.right - baseRect.left
};
return new window.DOMRect(
Copy link
Contributor Author

@straker straker Aug 2, 2023

Choose a reason for hiding this comment

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

I thought it was weird that split rects works on DOMRects but then doesn't return one, which meant that doing splitRect(rect1, rect2).left would return undefined instead of the x value.

`);
const nodeA = fixture.children[1];
const nodeB = fixture.children[3];
assert.closeTo(getOffset(nodeA, nodeB), 38, round);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the reason that this isn't exactly 40 because you're using buttons? This is why I used links when I wrote this originally, no min space, no padding, no border. I think you should use links so we get more consistent / predictable numbers.

Copy link
Contributor Author

@straker straker Aug 4, 2023

Choose a reason for hiding this comment

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

It's because the distance between the centers of both buttons gets subtracted by the circle radius (50 - 12).

lib/commons/dom/get-target-size.js Outdated Show resolved Hide resolved
lib/commons/dom/get-target-size.js Outdated Show resolved Hide resolved
lib/checks/mobile/target-offset-evaluate.js Outdated Show resolved Hide resolved
lib/commons/math/get-offset.js Outdated Show resolved Hide resolved
test/commons/math/get-offset.js Outdated Show resolved Hide resolved
test/commons/math/get-offset.js Outdated Show resolved Hide resolved
lib/commons/math/get-offset.js Outdated Show resolved Hide resolved
test/commons/math/get-offset.js Outdated Show resolved Hide resolved
test/commons/math/split-rects.js Outdated Show resolved Hide resolved
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.

Needs updated integration tests

@straker
Copy link
Contributor Author

straker commented Aug 7, 2023

Created #4119 and #4120

@straker straker merged commit 49eaa0e into develop Aug 8, 2023
3 checks passed
@straker straker deleted the target-size-update branch August 8, 2023 14:47
@adamraine adamraine mentioned this pull request Sep 6, 2023
1 task
andrewnicols added a commit to andrewnicols/moodle that referenced this pull request Jul 17, 2024
Disable the target-size rule.
This rule was added in Axe 4.7, but changed in 4.8 to allow for spacing
around the element. See dequelabs/axe-core#4117
for more information on this change.

Moodle 4.2 has an earlier version of Axe which does not have this rule.
Moodle 4.4 has a later, unaffected, version of Axe.

The simplest solution here is to simply disable the buggy rule.
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.

Update target-size to the latest WCAG spec
2 participants