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): correctly calculate bounding box #4125

Merged
merged 8 commits into from
Aug 21, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 9, 2023

The integration tests had to be updated with this change. The passing examples were failing as the target circle was indeed overlapping the other target (just barely). I updated them with a larger distance so they would still pass for the first 2 cases.

Closes: #4119

@straker straker requested a review from a team as a code owner August 9, 2023 20:18
dbjorge
dbjorge previously requested changes Aug 9, 2023
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks @straker ! Some notes inline

* @return {DOMRect[]}
*/
function getTargetRects(vNode) {
const nodeRect = vNode.boundingClientRect;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will eventually need to be more complicated in order to address the issue @WilcoFiers pointed out the other day where boundingClientRect doesn't take into account cases where a target has a descendant element that's part of the target for the purposes of the SC, but which overflow the bounding box of the target (eg, a link with an inline image that exceeds the link's line height).

I think it's okay for this PR to not address that, but it'd be good to file a new tech debt item to track it and add a breadcrumb to the issue here.

Copy link
Contributor

Choose a reason for hiding this comment

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

#3673 is closely related, but the fix that closed it only covered the target-size check, I think the target-offset check is still vulnerable to the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this as part of #4120

lib/commons/math/get-offset.js Outdated Show resolved Hide resolved
*/
function getTargetRects(vNode) {
const nodeRect = vNode.boundingClientRect;
const overlappingVNodes = findNearbyElms(vNode).filter(vNeighbor => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moved and not written in this PR, but I'm noticing as I'm looking at it that it's probably incorrectly considering non-separate-target-descendants as obscuring the target rather than being part of the target. I think this needs to be more complex, along the same lines as filterByElmsOverlap in target-size-evaluate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this as part of #4120

dbjorge
dbjorge previously approved these changes Aug 9, 2023
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

LGTM modulo the work deferred for #4120, but would be good to have @WilcoFiers 's eyes on it too

test/integration/full/target-size/target-size.html Outdated Show resolved Hide resolved
lib/commons/dom/get-target-rects.js Outdated Show resolved Hide resolved
lib/commons/dom/get-target-rects.js 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.

Update math.getOffset algorithm to better align with spec
3 participants