-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(modal): remove safe-area gap and flash in fullscreen modals #31092
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
Changes from 4 commits
a350f04
05ce997
5c59d59
01200b9
559103a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,11 @@ import { configs, test, Viewports } from '@utils/test/playwright'; | |
| * The test page (index.html) sets these root safe-area values. | ||
| * Keep in sync with the :root block in test/safe-area/index.html. | ||
| */ | ||
| const TEST_SAFE_AREA_TOP = '47px'; | ||
| const TEST_SAFE_AREA_TOP = 47; | ||
| const TEST_SAFE_AREA_BOTTOM = 34; | ||
| /** Default value of --ion-padding (16px), applied via the .ion-padding class on ion-content in the test modal. */ | ||
| const TEST_ION_PADDING = 16; | ||
|
|
||
| configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, config }) => { | ||
| test.describe(title('modal: safe-area handling'), () => { | ||
| test.beforeEach(async ({ page }) => { | ||
|
|
@@ -100,10 +104,12 @@ configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, config | |
| expect(safeAreaBottom).toBe('inherit'); | ||
| }); | ||
|
|
||
| test('fullscreen modal without footer should have wrapper padding-bottom', async ({ page }, testInfo) => { | ||
| test('fullscreen modal without footer should set safe-area scroll padding on ion-content', async ({ | ||
| page, | ||
| }, testInfo) => { | ||
| testInfo.annotations.push({ | ||
| type: 'issue', | ||
| description: 'https://github.com/ionic-team/ionic-framework/issues/30900', | ||
| description: 'https://github.com/ionic-team/ionic-framework/issues/31015', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was the issue changed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old version of this test asserted on |
||
| }); | ||
|
|
||
| const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent'); | ||
|
|
@@ -113,20 +119,83 @@ configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, config | |
|
|
||
| const modal = page.locator('ion-modal'); | ||
|
|
||
| // When no footer is present, the wrapper should have reduced height | ||
| // and padding-bottom to prevent content from overlapping the system | ||
| // navigation bar, without changing box-sizing (which would break | ||
| // custom modals with --border-width). | ||
| // The wrapper should NOT have reduced height or padding-bottom. | ||
| // Safe-area compensation is handled by ion-content's scroll padding. | ||
| const wrapper = modal.locator('.modal-wrapper'); | ||
| const paddingBottom = await wrapper.evaluate((el: HTMLElement) => { | ||
| const wrapperPaddingBottom = await wrapper.evaluate((el: HTMLElement) => { | ||
| return el.style.getPropertyValue('padding-bottom'); | ||
| }); | ||
| const height = await wrapper.evaluate((el: HTMLElement) => { | ||
| const wrapperHeight = await wrapper.evaluate((el: HTMLElement) => { | ||
| return el.style.getPropertyValue('height'); | ||
| }); | ||
|
|
||
| expect(paddingBottom).toBe('var(--ion-safe-area-bottom, 0px)'); | ||
| expect(height).toBe('calc(var(--height) - var(--ion-safe-area-bottom, 0px))'); | ||
| expect(wrapperPaddingBottom).toBe(''); | ||
| expect(wrapperHeight).toBe(''); | ||
|
|
||
| // ion-content should have --ion-content-safe-area-padding-bottom set so its | ||
| // .inner-scroll element includes safe-area in its bottom padding. | ||
| const content = modal.locator('ion-content'); | ||
| const safeAreaPadding = await content.evaluate((el: HTMLElement) => { | ||
| return el.style.getPropertyValue('--ion-content-safe-area-padding-bottom'); | ||
| }); | ||
| expect(safeAreaPadding).toBe('var(--ion-safe-area-bottom, 0px)'); | ||
| }); | ||
|
|
||
| test('fullscreen modal with ion-content and no footer should not reduce wrapper content area', async ({ | ||
| page, | ||
| }, testInfo) => { | ||
| testInfo.annotations.push({ | ||
| type: 'issue', | ||
| description: 'https://github.com/ionic-team/ionic-framework/issues/31015', | ||
| }); | ||
|
|
||
| const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent'); | ||
|
|
||
| await page.click('#fullscreen-modal-no-footer'); | ||
| await ionModalDidPresent.next(); | ||
|
|
||
| const modal = page.locator('ion-modal'); | ||
| const wrapper = modal.locator('.modal-wrapper'); | ||
|
|
||
| // The wrapper's content area should equal the full viewport height. | ||
| // Safe-area compensation is handled by ion-content's scroll padding, | ||
| // not by reducing the wrapper. This prevents the visible white gap | ||
| // reported in #31015. | ||
| const { contentHeight, paddingBottom } = await wrapper.evaluate((el: HTMLElement) => { | ||
| const computed = getComputedStyle(el); | ||
| return { | ||
| contentHeight: parseFloat(computed.height), | ||
| paddingBottom: parseFloat(computed.paddingBottom), | ||
| }; | ||
| }); | ||
| const viewportHeight = await page.evaluate(() => window.innerHeight); | ||
|
|
||
| expect(paddingBottom).toBeCloseTo(0, 0); | ||
| expect(contentHeight).toBeCloseTo(viewportHeight, 0); | ||
| }); | ||
|
|
||
| test('fullscreen modal ion-content scroll padding should include safe-area-bottom', async ({ page }, testInfo) => { | ||
| testInfo.annotations.push({ | ||
| type: 'issue', | ||
| description: 'https://github.com/ionic-team/ionic-framework/issues/31015', | ||
| }); | ||
|
|
||
| const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent'); | ||
|
|
||
| await page.click('#fullscreen-modal-no-footer'); | ||
| await ionModalDidPresent.next(); | ||
|
|
||
| const modal = page.locator('ion-modal'); | ||
| const content = modal.locator('ion-content'); | ||
|
|
||
| // The .inner-scroll element inside ion-content's shadow DOM should | ||
| // have padding-bottom that includes the safe-area-bottom value. | ||
| const innerScroll = content.locator('.inner-scroll'); | ||
| const scrollPaddingBottom = await innerScroll.evaluate((el: Element) => { | ||
| return parseFloat(getComputedStyle(el).paddingBottom); | ||
| }); | ||
|
|
||
| expect(scrollPaddingBottom).toBe(TEST_ION_PADDING + TEST_SAFE_AREA_BOTTOM); | ||
| }); | ||
|
|
||
| test('sheet modal at breakpoint 1 should keep top safe-area zeroed', async ({ page }, testInfo) => { | ||
|
|
@@ -185,7 +254,7 @@ configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, config | |
| const offsetTop = await modal.evaluate((el: HTMLIonModalElement) => { | ||
| return el.style.getPropertyValue('--ion-modal-offset-top'); | ||
| }); | ||
| expect(offsetTop).toBe(TEST_SAFE_AREA_TOP); | ||
| expect(offsetTop).toBe(`${TEST_SAFE_AREA_TOP}px`); | ||
| }); | ||
|
|
||
| test('fullscreen modal safe-area should update on resize from phone to tablet', async ({ page }, testInfo) => { | ||
|
|
@@ -251,6 +320,35 @@ configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, config | |
| expect(safeAreaBottom).toBe('0px'); | ||
| }); | ||
|
|
||
| test('centered dialog with custom dimensions on phone should zero safe-area from initial prediction', async ({ | ||
| page, | ||
| }, testInfo) => { | ||
| testInfo.annotations.push({ | ||
| type: 'issue', | ||
| description: 'https://github.com/ionic-team/ionic-framework/issues/31015', | ||
| }); | ||
|
|
||
| // Stay on phone viewport. This is the path where the centered-dialog | ||
| // media query does NOT match but the modal still doesn't touch screen | ||
| // edges because cssClass sets --width/--height. Without the initial | ||
| // prediction catching this, safe-area flashes inherited values and | ||
| // then snaps to 0px after animation. | ||
| const ionModalWillPresent = await page.spyOnEvent('ionModalWillPresent'); | ||
| await page.click('#centered-dialog'); | ||
| await ionModalWillPresent.next(); | ||
|
|
||
| // Read inline style IMMEDIATELY after will-present fires, before the | ||
| // animation finishes. This captures the initial prediction value. | ||
| const modal = page.locator('ion-modal'); | ||
| const initial = await modal.evaluate((el: HTMLIonModalElement) => ({ | ||
| top: el.style.getPropertyValue('--ion-safe-area-top'), | ||
| bottom: el.style.getPropertyValue('--ion-safe-area-bottom'), | ||
| })); | ||
|
|
||
| expect(initial.top).toBe('0px'); | ||
| expect(initial.bottom).toBe('0px'); | ||
| }); | ||
|
|
||
| test('safe-area overrides should be cleared on dismiss', async ({ page }, testInfo) => { | ||
| testInfo.annotations.push({ | ||
| type: 'issue', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong but it seems that
--ion-content-safe-area-padding-bottomis a a new variable to the project? If that's the case, can we comment that it's meant for internal usages and when to use it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! Done: 559103a