Skip to content

fix(sheet): move sheet footers instead of cloning while dragging #30433

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

Merged
merged 13 commits into from
Jun 3, 2025
Merged

Conversation

ShaneK
Copy link
Member

@ShaneK ShaneK commented May 27, 2025

Issue number: resolves several (listed at the bottom) + internal


What is the current behavior?

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?

We are now following method 2 of the design doc, 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
  • No

Other information

This refactor in how the footer combats shakiness should resolve many issues caused by the previous implementation, including the following:

Current dev build: 8.5.8-dev.11748530383.18b4e301

@ShaneK ShaneK requested a review from a team as a code owner May 27, 2025 16:45
@ShaneK ShaneK requested a review from OS-paulvisciano May 27, 2025 16:45
Copy link

vercel bot commented May 27, 2025

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 Jun 3, 2025 3:33pm

@github-actions github-actions bot added the package: core @ionic/core package label May 27, 2025
@ShaneK ShaneK changed the title fix(sheet): changing the way sheet dragging works to fix several issues that come from cloned footers fix(sheet): changing the way sheet footers work during dragging to fix several issues that come from cloned footers May 27, 2025
@ShaneK ShaneK changed the title fix(sheet): changing the way sheet footers work during dragging to fix several issues that come from cloned footers fix(sheet): changing the way sheet footers work during dragging May 27, 2025
is not duplicated, but the position is changed instead
@kumibrr
Copy link
Contributor

kumibrr commented May 28, 2025

@ShaneK Thanks for the heads-up!

I had tried a similar approach but ran into issues on larger viewports:

Screen.Recording.2025-05-28.at.09.39.43.mov

While investigating the bug, I attempted to address it by setting the modal’s width as the max-width of the footer and using transform: translateX. However, I struggled to get the right transform value.

Additionally, we must apply padding to the footer’s last toolbar to account for the bottom safe area.

@ShaneK
Copy link
Member Author

ShaneK commented May 28, 2025

Hey, I greatly appreciate that, can't believe I didn't catch it! Let me do some more investigating today and I'll see what I can come up with. Thank you!

@ShaneK
Copy link
Member Author

ShaneK commented May 28, 2025

Hey @kumibrr! I've attempted to address this by pinning the constraints of the footer while it's being dragged. Can you please try again and see if you can identify any issues with this approach?

I know you just used the test page for the flaw you found before, but if you want to try on a different project, there's a new dev build: 8.5.8-dev.11748450896.16232653

@kumibrr
Copy link
Contributor

kumibrr commented May 29, 2025

@ShaneK Yeah, I've noticed that the jittering is back on Safari and lower-end devices using chrome. This is caused because somehow the footer element is positioned relative to the parent. To better debug it, turn on CPU throttling in your browser dev tools as Chrome's swiftness hides the issue.

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

@ShaneK Yeah, I've noticed that the jittering is back on Safari and lower-end devices using chrome. This is caused because somehow the footer element is positioned relative to the parent. To better debug it, turn on CPU throttling in your browser dev tools as Chrome's swiftness hides the issue.

Is this after you drag and then let go and it's still animating? That's a known issue and it occurs with the current implementation too

@kumibrr
Copy link
Contributor

kumibrr commented May 29, 2025

@ShaneK Exactly.

I've tried with the current implementation and it doesn't happen. The cloned footer was created to solve that exact issue. Is there any instance where it does happen I missed?

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

@ShaneK Exactly.

I've tried with the current implementation and it doesn't happen. The cloned footer was created to solve that exact issue. Is there any instance where it does happen I missed?

When you're dragging and you let go and it's still animating it swaps back, though I did notice sometimes it doesn't swap back correctly so it may not always happen. Does it jitter with this implementation when you're holding it too or only after you let go? It doesn't make sense that this could be bound to the location of the parent since it's moved to the body.

@kumibrr
Copy link
Contributor

kumibrr commented May 29, 2025

@ShaneK
It only happens when I release. If I'm holding it, works beautifully.

Perhaps the swap to stationary should be executed in the animation.onFinish, because executing it on the moveSheetToBreakpoint swaps the footer before the animation even starts.

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

@ShaneK

It only happens when I release. If I'm holding it, works beautifully.

Perhaps the swap to stationary should be executed in the animation.onFinish, because executing it on the moveSheetToBreakpoint swaps the footer before the animation even starts.

Yeah, I've actually spent a lot of time troubleshooting this specific issue. You can't swap only on finish because then the footer won't collapse correctly. I address this while it's being moved by swapping back while dragging if we cross below the height of the footer, however onMove doesn't trigger after release because it only triggers as part of the drag gesture. We have a spike to investigate a solution to this, but since it's an existing problem we considered it acceptable for this change which addresses so many other issues.

@kumibrr
Copy link
Contributor

kumibrr commented May 29, 2025

@ShaneK

Can we change the condition so it sets it as stationary only when the snapToBreakpoint is zero?
That way, the footer collapses correctly, and if the animation snaps to a breakpoint that's not zero, the footer will stay in the moving position until the animation ends.

...
    /**
     * If expandToScroll is disabled, we need to swap
     * the footer position to stationary so that it
     * will act as it would by default
     */
    if (!expandToScroll && snapToBreakpoint === 0) {
      swapFooterPosition('stationary');
    }

    return new Promise<void>((resolve) => {
      animation
        .onFinish(
          () => {
            if (shouldRemainOpen) {
              swapFooterPosition('stationary');
...

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

Hey, that's a really good idea! I'll try it out when I'm at my computer in a bit, thanks!

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

I've made that change and it seems to work pretty well! I also found and fixed another issue where toolbars weren't being considered when dragging, so now the toolbar height (if it exists) is taken into consideration while dragging and the footer will collapse/expand properly before they overlap. Thanks for that suggestion!

New dev build: 8.5.8-dev.11748525800.1cdc649b

@ShaneK
Copy link
Member Author

ShaneK commented May 29, 2025

I forgot to add the if statement in the onFinish callback so I just pushed another change to add it back. It should have had basically no bearing on anything ever that I can think of, but better safe than sorry. New dev build with this minor change, and hopefully it's the last until we go through full review with the team:
8.5.8-dev.11748530383.18b4e301

@ShaneK ShaneK requested review from brandyscarney and thetaPC and removed request for OS-paulvisciano May 29, 2025 15:28
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM, all issues have been fixed! Great job!

Co-authored-by: Brandy Smith <[email protected]>
@ShaneK ShaneK changed the title fix(sheet): changing the way sheet footers work during dragging fix(sheet): changing sheet footers to be moved instead of cloned while being dragged Jun 3, 2025
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Looks good! Tested all resolved issues and everything works well. Great work on this @ShaneK and @kumibrr! 🚀

@ShaneK ShaneK added this pull request to the merge queue Jun 3, 2025
@ShaneK ShaneK removed this pull request from the merge queue due to a manual request Jun 3, 2025
@ShaneK ShaneK changed the title fix(sheet): changing sheet footers to be moved instead of cloned while being dragged fix(sheet): move sheet footers instead of cloning while dragging Jun 3, 2025
@ShaneK ShaneK added this pull request to the merge queue Jun 3, 2025
Merged via the queue into main with commit 4cbbbb0 Jun 3, 2025
52 checks passed
@ShaneK ShaneK deleted the FW-6519 branch June 3, 2025 15:58
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
4 participants