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(popover): adjust position to account for approximate safe area #29068

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

averyjohnston
Copy link
Contributor

@averyjohnston averyjohnston commented Feb 16, 2024

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 main doesn'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 of side, 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 adjustment test, 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?

  • Yes
  • No

Other information

@averyjohnston averyjohnston changed the title Fw 5463 fix(popover): adjust position to account for approximate safe area Feb 16, 2024
@github-actions github-actions bot added the package: core @ionic/core package label Feb 16, 2024
Comment on lines -125 to -135
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}`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I understand: Since we are approximating the safe area bounds we no longer need these checks since it's already handled elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right; the adjustment is all done in calculateWindowAdjustment now.

@averyjohnston averyjohnston marked this pull request as ready for review February 16, 2024 19:44
@thetaPC
Copy link
Contributor

thetaPC commented Feb 28, 2024

@amandaejohnston is this ready for review? I noticed that a screenshot test is failing.

@averyjohnston
Copy link
Contributor Author

@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);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be screenshot tests according to our best practices guide.

(same as below test)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines -125 to -135
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}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we write a test for this?

Base automatically changed from feature-7.8 to main March 13, 2024 13:41
@averyjohnston
Copy link
Contributor Author

Closing for now due to shifting priorities. This may be reopened later after we've had a chance to discuss the strategy internally.

@averyjohnston averyjohnston reopened this Apr 16, 2024
@averyjohnston averyjohnston marked this pull request as draft April 16, 2024 15:45
Copy link

vercel bot commented Apr 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 16, 2024 4:07pm

Base automatically changed from feature-8.1 to main May 1, 2024 14:57
@sean-perkins sean-perkins removed their request for review May 22, 2024 14:01
@Webrow
Copy link

Webrow commented Sep 26, 2024

I am unsure of how to read this thread with the vercel bot inbetween.
Has this been merged and released yet? Or is this still waiting for steps.
We are currently on 8.2.7. and still have the bottom and top of the popover list "falling off the screen"

@thetaPC
Copy link
Contributor

thetaPC commented Sep 26, 2024

Hello @Webrow!

The issue still exists within Ionic Framework. I would recommend subscribing to its GitHub issue to receive updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: popover positioning does not properly account for safe area
5 participants