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

Position calculation is sus when the floating element is larger than viewport. #244

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

pksjce
Copy link
Contributor

@pksjce pksjce commented Nov 14, 2023

While debugging https://github.com/github/accessibility-audits/issues/4515 , I was able to narrow down the issue to this criteria.

When zoomed in, the floating element becomes of larger in height compared to the viewport. This results in negative top value in the anchor position calculation. Thus results in top parts of the floating element being cut off.
Before I can try to fix this by messing with the super complex calculations, I thought I'd float a test case to see if my assumptions are right.

I tried to keep the test case values as close to the real life use case as possible. You can see that the height of the floating element is 428px and the height of the viewport is only 400px. This already results in a negative top value which is buggy as far as I understand.

Open to feedback

Copy link

changeset-bot bot commented Nov 14, 2023

⚠️ No Changeset found

Latest commit: 4885192

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@keithamus keithamus added the skip changeset Skip the check for changesets label Nov 14, 2023
keithamus
keithamus previously approved these changes Nov 14, 2023
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This test looks good. Are you intending to merge-as-is and fix the test+code in a followup? Or would you like to continue this PR with a fix?

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

We paired on this together, so should get another review as well 🤭

@pksjce
Copy link
Contributor Author

pksjce commented Nov 14, 2023

@keithamus - Just paired with @siddharthkp and added a possible fix. Tested it on primer-react as well and it fixes the use case while fixing the new test case also.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

LGTM still 👍

@pksjce pksjce merged commit b0e58a0 into main Nov 15, 2023
9 checks passed
@pksjce pksjce deleted the pk/zoom_overlay_position branch November 15, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset Skip the check for changesets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants