fix(popover): adjust position to account for approximate safe area #29068
Conversation
averyjohnston
changed the title
| const safeAreaLeft = ' + var(--ion-safe-area-left, 0)'; | ||
| const safeAreaRight = ' - var(--ion-safe-area-right, 0)'; | ||
|
|
||
| let leftValue = `${left}px`; | ||
|
|
||
| if (checkSafeAreaLeft) { | ||
| leftValue = `${left}px${safeAreaLeft}`; | ||
| } | ||
| if (checkSafeAreaRight) { | ||
| leftValue = `${left}px${safeAreaRight}`; | ||
| } |
There was a problem hiding this comment.
While removing these looks like a step backwards at a glance, they weren't actually working as intended because the checkSafeAreaLeft/Right variables were based on the hardcoded 0/25px guess at the margins. I've removed them to ensure the MD and iOS safe area behavior lines up as closely as possible.
There was a problem hiding this comment.
So I understand: Since we are approximating the safe area bounds we no longer need these checks since it's already handled elsewhere?
There was a problem hiding this comment.
Right; the adjustment is all done in calculateWindowAdjustment now.
averyjohnston
requested review from
liamdebeasi,
sean-perkins and
a team
as code owners
|
@amandaejohnston is this ready for review? I noticed that a screenshot test is failing. |
|
@thetaPC Fixed the screenshots 👍 This is indeed ready for review. |
| * of the screen height. We use 10px as the threshold to give | ||
| * wiggle room and help prevent flakiness. | ||
| */ | ||
| expect(box.x < 10).toBe(true); |
There was a problem hiding this comment.
These should be screenshot tests according to our best practices guide.
(same as below test)
There was a problem hiding this comment.
This doesn't look correct. In the old screenshot the arrow is aligned with the popover body for the top-most popover. That's no longer true in the new screenshot.
| const safeAreaLeft = ' + var(--ion-safe-area-left, 0)'; | ||
| const safeAreaRight = ' - var(--ion-safe-area-right, 0)'; | ||
|
|
||
| let leftValue = `${left}px`; | ||
|
|
||
| if (checkSafeAreaLeft) { | ||
| leftValue = `${left}px${safeAreaLeft}`; | ||
| } | ||
| if (checkSafeAreaRight) { | ||
| leftValue = `${left}px${safeAreaRight}`; | ||
| } |
There was a problem hiding this comment.
So I understand: Since we are approximating the safe area bounds we no longer need these checks since it's already handled elsewhere?
| */ | ||
| let horizontalSafeAreaApprox = 0; | ||
| let verticalSafeAreaApprox = 0; | ||
| if (win?.matchMedia !== undefined) { |
There was a problem hiding this comment.
matchMedia should always be defined in the supported browsers. Are there other scenarios where this is not defined?
| * adjust top margins. | ||
| */ | ||
| if (side === 'top' || side === 'bottom') { | ||
| top = Math.max(top, verticalSafeAreaApprox + bodyPadding); |
There was a problem hiding this comment.
Can we write a test for this?
| /** | ||
| * Ensure the popover doesn't sit below the safe area approxmation. | ||
| */ | ||
| top = Math.min(top, bodyHeight - verticalSafeAreaApprox - contentHeight - bodyPadding); |
There was a problem hiding this comment.
Can we write a test for this?
|
Closing for now due to shifting priorities. This may be reopened later after we've had a chance to discuss the strategy internally. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I am unsure of how to read this thread with the vercel bot inbetween. |
Issue number: resolves #28411
What is the current behavior?
Popover position does not account for safe area. iOS has some existing logic that attempts to do so, but it always uses a hardcoded value of 25px for the margin (or 0 for
size="cover"popovers).What is the new behavior?
The most straightforward way of accounting for safe area would be to use
window.getComputedStyle()to calculate the safe area margins when the popover's position is being determined. However, this function is very expensive, so using it would introduce performance issues. Until a better strategy can be found that properly accounts for all cases, this PR approximates things by making a guess at the safe area margins.Note that the position of the arrow on iOS popovers has not been adjusted to account for safe area in the same way as the popover content. I chose to leave it like this because the behavior in
maindoesn't adjust the arrow either, even with the 25px guess at the safe area margin. Moving the arrow will also add complexity to account for the different values ofside, which increases the risk of bugs. If it does need fixing, it would probably be best to do that as separate scope.This PR has been targeted to a minor release to de-risk the behavior change.
The easiest way to test the behavior is with the
adjustmenttest, as this lets you present the popover from anywhere on the screen. Checking the new "show safe area approximation" checkbox highlights the safe area margin guesses in blue.Does this introduce a breaking change?
Other information