Ensure sibling Dialog components are scrollable on mobile
#3796
+29
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is just a very very small optimization, but wanted to optimize it since I noticed it. Essentially `containers.flatMap(resolve => resolve())` is a bit wasteful if we you have `n` containers, but you already found a match in the first container. If that's the case, then we resolved `n-1` containers for nothing. Depending on what we actually do during this resolve step it could be a lot of unnecessary work (e.g.: querying the DOM).
Otherwise we have to build it on every scroll event and that would be very expensive.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
RobinMalfait
commented
Sep 17, 2025
Comment on lines
+77
to
+79
| meta() { | ||
| return entry.computedMeta | ||
| }, |
There was a problem hiding this comment.
I considered making this a getter, but then you have to use entry.meta instead of destructuring it to prevent the meta to be evaluated already.
| // that happens later can update the meta information. Otherwise we would | ||
| // use stale meta information. | ||
| meta() { | ||
| return entry.computedMeta |
There was a problem hiding this comment.
Used entry instead of destructuring the computedMeta to ensure we are looking at the latest data. Alternatively we could use this.get(doc).computedMeta or something
There was a problem hiding this comment.
I know this kind of sounds like the opposite of the above message, but the difference is a getter here vs destructuring in another spot so the connection might not be as clear.
thecrypticace
approved these changes
Sep 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where opening a sibling dialog doesn't allow you to scroll the second (top most) dialog on iOS anymore.
The issue here is that on iOS we have to do a lot of annoying work to make the dialog behave like a dialog and prevent scrolling outside of the dialog.
To make this all work, we PUSH and POP some information to know what the top-most dialog is. The moment there is 1 open dialog, and as long as there is 1 dialog we scroll lock the page, the moment there are 0 dialogs again we unlock the page again.
Each dialog also has a set of "allowed containers", these are just container DOM nodes where we allow scrolling.
But the issue is that we were dealing with stale data. These are the events you see when we open dialogs:
Any time we
PUSHwe also track new meta data with the allowed containers. Butwe were already preventing scroll, so we had stale meta data.
To solve this, I can think of 2 solutions:
Test plan
In both scenario's I first open the dialog, then scroll.
before:
ScreenRecording_09-17-2025.17-18-13_1.MP4
after:
ScreenRecording_09-17-2025.17-18-35_1.MP4
Fixes: #3378