Skip to content

Commit 4cbbbb0

Browse files
ShaneKkumibrrthetaPCbrandyscarney
authored
fix(sheet): move sheet footers instead of cloning while dragging (#30433)
Issue number: resolves several (listed at the bottom) + internal --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> Currently, when expand to scroll is disabled in a sheet modal, we duplicate the footer on drag and show a cloned version in the shadow DOM instead of the original. This causes many issues (described in the Other Information section), especially because often times the cloned version of the footer would stick around instead of the original version, which made any event listeners in the footer (and styling) broken by default instead of only while dragging. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> We are now following [method 2 of the design doc](https://github.com/ionic-team/ionic-framework-design-documents/blob/main/projects/ionic-framework/components/modal/0003-sheet-modal-scroll.md#approach-2-move-the-footer), with minor deviation. Now we try to eliminate the shaky behavior in the footer by setting the footer to be absolutely positioned on the page and adding padding to the bottom of the modal while dragging is happening to offset the missing footer content. We are additionally performing some extra logic to swap back when the modal is dragged below the height of the footer so that it collapses correctly visually. Note this is a minor variation in method two from the design doc because I moved the footer to the body instead of to the parent modal. This is because the parent modal will prevent anything not in its ion-page from displaying, but it seems to work fine. As a side-effect of this, I had to add an extra class and support for that class in a few areas so we could identify footers that belonged to modals while they were moving and apply applicable classes. Additionally with this change I was able to remove all of the extra code in the leave/enter animations because we no longer need to worry about managing a clone of an element there. This allows me to contain all code relating to behavior of the footer to `sheet.ts`. ## Does this introduce a breaking change? - [ ] Yes - [X] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information This refactor in how the footer combats shakiness should resolve many issues caused by the previous implementation, including the following: - this change no longer has the footer being swapped out, effectively breaking event listeners, which fixes #30315 - this change no longer has the footer being duplicated at all, which fixes #30341 - this change no longer has the footer (sometimes) being moved to the shadow DOM, which fixes #30312 **Current dev build**: `8.5.8-dev.11748530383.18b4e301` --------- Co-authored-by: Israel de la Barrera <[email protected]> Co-authored-by: Maria Hutt <[email protected]> Co-authored-by: Brandy Smith <[email protected]>
1 parent c62590a commit 4cbbbb0

File tree

7 files changed

+115
-195
lines changed

7 files changed

+115
-195
lines changed

‎core/src/components/modal/animations/ios.enter.ts

Lines changed: 1 addition & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -41,50 +41,7 @@ export const iosEnterAnimation = (baseEl: HTMLElement, opts: ModalAnimationOptio
4141
.addElement(baseEl)
4242
.easing('cubic-bezier(0.32,0.72,0,1)')
4343
.duration(500)
44-
.addAnimation([wrapperAnimation])
45-
.beforeAddWrite(() => {
46-
if (expandToScroll) {
47-
// Scroll can only be done when the modal is fully expanded.
48-
return;
49-
}
50-
51-
/**
52-
* There are some browsers that causes flickering when
53-
* dragging the content when scroll is enabled at every
54-
* breakpoint. This is due to the wrapper element being
55-
* transformed off the screen and having a snap animation.
56-
*
57-
* A workaround is to clone the footer element and append
58-
* it outside of the wrapper element. This way, the footer
59-
* is still visible and the drag can be done without
60-
* flickering. The original footer is hidden until the modal
61-
* is dismissed. This maintains the animation of the footer
62-
* when the modal is dismissed.
63-
*
64-
* The workaround needs to be done before the animation starts
65-
* so there are no flickering issues.
66-
*/
67-
const ionFooter = baseEl.querySelector('ion-footer');
68-
/**
69-
* This check is needed to prevent more than one footer
70-
* from being appended to the shadow root.
71-
* Otherwise, iOS and MD enter animations would append
72-
* the footer twice.
73-
*/
74-
const ionFooterAlreadyAppended = baseEl.shadowRoot!.querySelector('ion-footer');
75-
if (ionFooter && !ionFooterAlreadyAppended) {
76-
const footerHeight = ionFooter.clientHeight;
77-
const clonedFooter = ionFooter.cloneNode(true) as HTMLIonFooterElement;
78-
79-
baseEl.shadowRoot!.appendChild(clonedFooter);
80-
ionFooter.style.setProperty('display', 'none');
81-
ionFooter.setAttribute('aria-hidden', 'true');
82-
83-
// Padding is added to prevent some content from being hidden.
84-
const page = baseEl.querySelector('.ion-page') as HTMLElement;
85-
page.style.setProperty('padding-bottom', `${footerHeight}px`);
86-
}
87-
});
44+
.addAnimation([wrapperAnimation]);
8845

8946
if (contentAnimation) {
9047
baseAnimation.addAnimation(contentAnimation);

‎core/src/components/modal/animations/ios.leave.ts

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ const createLeaveAnimation = () => {
1919
* iOS Modal Leave Animation
2020
*/
2121
export const iosLeaveAnimation = (baseEl: HTMLElement, opts: ModalAnimationOptions, duration = 500): Animation => {
22-
const { presentingEl, currentBreakpoint, expandToScroll } = opts;
22+
const { presentingEl, currentBreakpoint } = opts;
2323
const root = getElementRoot(baseEl);
2424
const { wrapperAnimation, backdropAnimation } =
2525
currentBreakpoint !== undefined ? createSheetLeaveAnimation(opts) : createLeaveAnimation();
@@ -32,33 +32,7 @@ export const iosLeaveAnimation = (baseEl: HTMLElement, opts: ModalAnimationOptio
3232
.addElement(baseEl)
3333
.easing('cubic-bezier(0.32,0.72,0,1)')
3434
.duration(duration)
35-
.addAnimation(wrapperAnimation)
36-
.beforeAddWrite(() => {
37-
if (expandToScroll) {
38-
// Scroll can only be done when the modal is fully expanded.
39-
return;
40-
}
41-
42-
/**
43-
* If expandToScroll is disabled, we need to swap
44-
* the visibility to the original, so the footer
45-
* dismisses with the modal and doesn't stay
46-
* until the modal is removed from the DOM.
47-
*/
48-
const ionFooter = baseEl.querySelector('ion-footer');
49-
if (ionFooter) {
50-
const clonedFooter = baseEl.shadowRoot!.querySelector('ion-footer')!;
51-
52-
ionFooter.style.removeProperty('display');
53-
ionFooter.removeAttribute('aria-hidden');
54-
55-
clonedFooter.style.setProperty('display', 'none');
56-
clonedFooter.setAttribute('aria-hidden', 'true');
57-
58-
const page = baseEl.querySelector('.ion-page') as HTMLElement;
59-
page.style.removeProperty('padding-bottom');
60-
}
61-
});
35+
.addAnimation(wrapperAnimation);
6236

6337
if (presentingEl) {
6438
const isMobile = window.innerWidth < 768;

‎core/src/components/modal/animations/md.enter.ts

Lines changed: 2 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -37,56 +37,13 @@ export const mdEnterAnimation = (baseEl: HTMLElement, opts: ModalAnimationOption
3737

3838
// The content animation is only added if scrolling is enabled for
3939
// all the breakpoints.
40-
expandToScroll && contentAnimation?.addElement(baseEl.querySelector('.ion-page')!);
40+
!expandToScroll && contentAnimation?.addElement(baseEl.querySelector('.ion-page')!);
4141

4242
const baseAnimation = createAnimation()
4343
.addElement(baseEl)
4444
.easing('cubic-bezier(0.36,0.66,0.04,1)')
4545
.duration(280)
46-
.addAnimation([backdropAnimation, wrapperAnimation])
47-
.beforeAddWrite(() => {
48-
if (expandToScroll) {
49-
// Scroll can only be done when the modal is fully expanded.
50-
return;
51-
}
52-
53-
/**
54-
* There are some browsers that causes flickering when
55-
* dragging the content when scroll is enabled at every
56-
* breakpoint. This is due to the wrapper element being
57-
* transformed off the screen and having a snap animation.
58-
*
59-
* A workaround is to clone the footer element and append
60-
* it outside of the wrapper element. This way, the footer
61-
* is still visible and the drag can be done without
62-
* flickering. The original footer is hidden until the modal
63-
* is dismissed. This maintains the animation of the footer
64-
* when the modal is dismissed.
65-
*
66-
* The workaround needs to be done before the animation starts
67-
* so there are no flickering issues.
68-
*/
69-
const ionFooter = baseEl.querySelector('ion-footer');
70-
/**
71-
* This check is needed to prevent more than one footer
72-
* from being appended to the shadow root.
73-
* Otherwise, iOS and MD enter animations would append
74-
* the footer twice.
75-
*/
76-
const ionFooterAlreadyAppended = baseEl.shadowRoot!.querySelector('ion-footer');
77-
if (ionFooter && !ionFooterAlreadyAppended) {
78-
const footerHeight = ionFooter.clientHeight;
79-
const clonedFooter = ionFooter.cloneNode(true) as HTMLIonFooterElement;
80-
81-
baseEl.shadowRoot!.appendChild(clonedFooter);
82-
ionFooter.style.setProperty('display', 'none');
83-
ionFooter.setAttribute('aria-hidden', 'true');
84-
85-
// Padding is added to prevent some content from being hidden.
86-
const page = baseEl.querySelector('.ion-page') as HTMLElement;
87-
page.style.setProperty('padding-bottom', `${footerHeight}px`);
88-
}
89-
});
46+
.addAnimation([backdropAnimation, wrapperAnimation]);
9047

9148
if (contentAnimation) {
9249
baseAnimation.addAnimation(contentAnimation);

‎core/src/components/modal/animations/md.leave.ts

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ const createLeaveAnimation = () => {
2121
* Md Modal Leave Animation
2222
*/
2323
export const mdLeaveAnimation = (baseEl: HTMLElement, opts: ModalAnimationOptions): Animation => {
24-
const { currentBreakpoint, expandToScroll } = opts;
24+
const { currentBreakpoint } = opts;
2525
const root = getElementRoot(baseEl);
2626
const { wrapperAnimation, backdropAnimation } =
2727
currentBreakpoint !== undefined ? createSheetLeaveAnimation(opts) : createLeaveAnimation();
@@ -32,33 +32,7 @@ export const mdLeaveAnimation = (baseEl: HTMLElement, opts: ModalAnimationOption
3232
const baseAnimation = createAnimation()
3333
.easing('cubic-bezier(0.47,0,0.745,0.715)')
3434
.duration(200)
35-
.addAnimation([backdropAnimation, wrapperAnimation])
36-
.beforeAddWrite(() => {
37-
if (expandToScroll) {
38-
// Scroll can only be done when the modal is fully expanded.
39-
return;
40-
}
41-
42-
/**
43-
* If expandToScroll is disabled, we need to swap
44-
* the visibility to the original, so the footer
45-
* dismisses with the modal and doesn't stay
46-
* until the modal is removed from the DOM.
47-
*/
48-
const ionFooter = baseEl.querySelector('ion-footer');
49-
if (ionFooter) {
50-
const clonedFooter = baseEl.shadowRoot!.querySelector('ion-footer')!;
51-
52-
ionFooter.style.removeProperty('display');
53-
ionFooter.removeAttribute('aria-hidden');
54-
55-
clonedFooter.style.setProperty('display', 'none');
56-
clonedFooter.setAttribute('aria-hidden', 'true');
57-
58-
const page = baseEl.querySelector('.ion-page') as HTMLElement;
59-
page.style.removeProperty('padding-bottom');
60-
}
61-
});
35+
.addAnimation([backdropAnimation, wrapperAnimation]);
6236

6337
return baseAnimation;
6438
};

‎core/src/components/modal/gestures/sheet.ts

Lines changed: 104 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ export const createSheetGesture = (
8484
let offset = 0;
8585
let canDismissBlocksGesture = false;
8686
let cachedScrollEl: HTMLElement | null = null;
87+
let cachedFooterEl: HTMLIonFooterElement | null = null;
88+
let cachedFooterYPosition: number | null = null;
89+
let currentFooterState: 'moving' | 'stationary' | null = null;
8790
const canDismissMaxStep = 0.95;
8891
const maxBreakpoint = breakpoints[breakpoints.length - 1];
8992
const minBreakpoint = breakpoints[0];
@@ -118,33 +121,74 @@ export const createSheetGesture = (
118121
};
119122

120123
/**
121-
* Toggles the visible modal footer when `expandToScroll` is disabled.
122-
* @param footer The footer to show.
124+
* Toggles the footer to an absolute position while moving to prevent
125+
* it from shaking while the sheet is being dragged.
126+
* @param newPosition Whether the footer is in a moving or stationary position.
123127
*/
124-
const swapFooterVisibility = (footer: 'original' | 'cloned') => {
125-
const originalFooter = baseEl.querySelector('ion-footer') as HTMLIonFooterElement | null;
126-
127-
if (!originalFooter) {
128-
return;
128+
const swapFooterPosition = (newPosition: 'moving' | 'stationary') => {
129+
if (!cachedFooterEl) {
130+
cachedFooterEl = baseEl.querySelector('ion-footer') as HTMLIonFooterElement | null;
131+
if (!cachedFooterEl) {
132+
return;
133+
}
129134
}
130135

131-
const clonedFooter = wrapperEl.nextElementSibling as HTMLIonFooterElement;
132-
const footerToHide = footer === 'original' ? clonedFooter : originalFooter;
133-
const footerToShow = footer === 'original' ? originalFooter : clonedFooter;
134-
135-
footerToShow.style.removeProperty('display');
136-
footerToShow.removeAttribute('aria-hidden');
137-
138-
const page = baseEl.querySelector('.ion-page') as HTMLElement;
139-
if (footer === 'original') {
140-
page.style.removeProperty('padding-bottom');
136+
const page = baseEl.querySelector('.ion-page') as HTMLElement | null;
137+
138+
currentFooterState = newPosition;
139+
if (newPosition === 'stationary') {
140+
// Reset positioning styles to allow normal document flow
141+
cachedFooterEl.classList.remove('modal-footer-moving');
142+
cachedFooterEl.style.removeProperty('position');
143+
cachedFooterEl.style.removeProperty('width');
144+
cachedFooterEl.style.removeProperty('height');
145+
cachedFooterEl.style.removeProperty('top');
146+
cachedFooterEl.style.removeProperty('left');
147+
page?.style.removeProperty('padding-bottom');
148+
149+
// Move to page
150+
page?.appendChild(cachedFooterEl);
141151
} else {
142-
const pagePadding = footerToShow.clientHeight;
143-
page.style.setProperty('padding-bottom', `${pagePadding}px`);
144-
}
152+
// Get both the footer and document body positions
153+
const cachedFooterElRect = cachedFooterEl.getBoundingClientRect();
154+
const bodyRect = document.body.getBoundingClientRect();
155+
156+
// Add padding to the parent element to prevent content from being hidden
157+
// when the footer is positioned absolutely. This has to be done before we
158+
// make the footer absolutely positioned or we may accidentally cause the
159+
// sheet to scroll.
160+
const footerHeight = cachedFooterEl.clientHeight;
161+
page?.style.setProperty('padding-bottom', `${footerHeight}px`);
162+
163+
// Apply positioning styles to keep footer at bottom
164+
cachedFooterEl.classList.add('modal-footer-moving');
165+
166+
// Calculate absolute position relative to body
167+
// We need to subtract the body's offsetTop to get true position within document.body
168+
const absoluteTop = cachedFooterElRect.top - bodyRect.top;
169+
const absoluteLeft = cachedFooterElRect.left - bodyRect.left;
170+
171+
// Capture the footer's current dimensions and hard code them during the drag
172+
cachedFooterEl.style.setProperty('position', 'absolute');
173+
cachedFooterEl.style.setProperty('width', `${cachedFooterEl.clientWidth}px`);
174+
cachedFooterEl.style.setProperty('height', `${cachedFooterEl.clientHeight}px`);
175+
cachedFooterEl.style.setProperty('top', `${absoluteTop}px`);
176+
cachedFooterEl.style.setProperty('left', `${absoluteLeft}px`);
177+
178+
// Also cache the footer Y position, which we use to determine if the
179+
// sheet has been moved below the footer. When that happens, we need to swap
180+
// the position back so it will collapse correctly.
181+
cachedFooterYPosition = absoluteTop;
182+
// If there's a toolbar, we need to combine the toolbar height with the footer position
183+
// because the toolbar moves with the drag handle, so when it starts overlapping the footer,
184+
// we need to account for that.
185+
const toolbar = baseEl.querySelector('ion-toolbar') as HTMLIonToolbarElement | null;
186+
if (toolbar) {
187+
cachedFooterYPosition -= toolbar.clientHeight;
188+
}
145189

146-
footerToHide.style.setProperty('display', 'none');
147-
footerToHide.setAttribute('aria-hidden', 'true');
190+
document.body.appendChild(cachedFooterEl);
191+
}
148192
};
149193

150194
/**
@@ -247,12 +291,11 @@ export const createSheetGesture = (
247291

248292
/**
249293
* If expandToScroll is disabled, we need to swap
250-
* the footer visibility to the original, so if the modal
251-
* is dismissed, the footer dismisses with the modal
252-
* and doesn't stay on the screen after the modal is gone.
294+
* the footer position to moving so that it doesn't shake
295+
* while the sheet is being dragged.
253296
*/
254297
if (!expandToScroll) {
255-
swapFooterVisibility('original');
298+
swapFooterPosition('moving');
256299
}
257300

258301
/**
@@ -275,6 +318,21 @@ export const createSheetGesture = (
275318
};
276319

277320
const onMove = (detail: GestureDetail) => {
321+
/**
322+
* If `expandToScroll` is disabled, we need to see if we're currently below
323+
* the footer element and the footer is in a stationary position. If so,
324+
* we need to make the stationary the original position so that the footer
325+
* collapses with the sheet.
326+
*/
327+
if (!expandToScroll && cachedFooterYPosition !== null && currentFooterState !== null) {
328+
// Check if we need to swap the footer position
329+
if (detail.currentY >= cachedFooterYPosition && currentFooterState === 'moving') {
330+
swapFooterPosition('stationary');
331+
} else if (detail.currentY < cachedFooterYPosition && currentFooterState === 'stationary') {
332+
swapFooterPosition('moving');
333+
}
334+
}
335+
278336
/**
279337
* If `expandToScroll` is disabled, and an upwards swipe gesture is done within
280338
* the scrollable content, we should not allow the swipe gesture to continue.
@@ -431,15 +489,6 @@ export const createSheetGesture = (
431489
*/
432490
gesture.enable(false);
433491

434-
/**
435-
* If expandToScroll is disabled, we need to swap
436-
* the footer visibility to the cloned one so the footer
437-
* doesn't flicker when the sheet's height is animated.
438-
*/
439-
if (!expandToScroll && shouldRemainOpen) {
440-
swapFooterVisibility('cloned');
441-
}
442-
443492
if (shouldPreventDismiss) {
444493
handleCanDismiss(baseEl, animation);
445494
} else if (!shouldRemainOpen) {
@@ -457,11 +506,31 @@ export const createSheetGesture = (
457506
contentEl.scrollY = true;
458507
}
459508

509+
/**
510+
* If expandToScroll is disabled and we're animating
511+
* to close the sheet, we need to swap
512+
* the footer position to stationary so that it
513+
* will collapse correctly. We cannot just always swap
514+
* here or it'll be jittery while animating movement.
515+
*/
516+
if (!expandToScroll && snapToBreakpoint === 0) {
517+
swapFooterPosition('stationary');
518+
}
519+
460520
return new Promise<void>((resolve) => {
461521
animation
462522
.onFinish(
463523
() => {
464524
if (shouldRemainOpen) {
525+
/**
526+
* If expandToScroll is disabled, we need to swap
527+
* the footer position to stationary so that it
528+
* will act as it would by default.
529+
*/
530+
if (!expandToScroll) {
531+
swapFooterPosition('stationary');
532+
}
533+
465534
/**
466535
* Once the snapping animation completes,
467536
* we need to reset the animation to go

0 commit comments

Comments
 (0)