fix: overlay layout shift when auto-focusing input#10102
Conversation
| preventScrollCount++; | ||
| if (preventScrollCount === 1) { | ||
| if (isIOS()) { | ||
| if (isIOS() && isWebKit()) { |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Does Popover.tsx have the same problem? I was hopeful this would fix the S2 Combobox which scrolls the main page and focuses the input when tapping the trigger, and the Popover ends up the wrong size in iOS26, but I was unsuccessful on a quick try.
In general we don't like to delay things by any sort of timer though know that they are sometimes unavoidable. What were some other avenues you explored?
Does this work with native elements that have autofocus? or only RAC components? React autoFocus timing is different from native is why I'm asking. I suspect it works for both given that the React one is "slower".
|
@snowystinger Is it #6609 (comment) you are referring to? Is there a screencast or reproduction available? Generally speaking though, yes, there's a number of other use cases for the same fix, e.g. scrollIntoView, popovers, etc. I limited the PR to Modal.tsx for now, since I wanted to get feedback before expanding the scope first. Long term, I think it still makes a lot of sense to rework useResizeObserver, as I tried hinting at before. There is a ton of complexity around observing a bound (scrollbar positioning, scrollbar thickness, viewport resizing, various interop problems, etc.), so I think there is a great case for a hook that supports both (layout/visual) viewport and element observation w/ support for different box-models. We will have to think of something that works outside of hooks though, likely with a global listener to track the OSK.
This change is really more of a feature than bugfix, or a courtesy if you will. Fwiw, we can't indefinitely follow focus events in the future, so we ought to make a best-guess attempt at when to measure. This inherently is a timer-based problem, so I don't think there is much else to explore. I chose the minimum of 2 frames to start the discussion, but there may be a case for a longer delay, e.g. to prevent the IOS top bar from sampling the wrong background color.
Yes, that should work fine, since the viewport resize event should always fire before the next frame. |
|
Yeah, it's just this where the url covers part of the results: No worries, I think limiting the PR to Modal.tsx for now is fine. I was just trying to see what else it could extend to or might affect.
Thanks for bringing it back to this, I hadn't considered it here. |
|
@snowystinger Converting this back to draft, and will push a superseding PR with the rework of useResizeObserver to discuss. We can decide from there which changes to take on and which to drop. |
|
@snowystinger The resize observation PR is taking too much of my time at the moment to wrap up, so I've split out the relevant parts here. This is now generic and fixes both Popover and Modal, including the mentioned viewport issue. Adoption can later trivially be expanded to other utilities, e.g. scrollIntoView. Also, I've decided to dial back on the opinionated styling, so the delayed reveal is now opt-in via a new data attribute. Most of the changes are docs, the interesting bits are in keyboard.tsx and runAfterKeyboard.ts. I will see whether I can also add tests once review is in 👍 |
nwidynski
changed the title
| transitionInterval = ownerWindow.setInterval(() => { | ||
| let isOpen = isKeyboardOpen(); | ||
| let isVisible = isKeyboardVisible(); | ||
|
|
||
| if (wasOpen !== isOpen) { | ||
| for (let callback of resizeCallbacks) { | ||
| callback(isKeyboardOpen()); | ||
| resizeCallbacks.delete(callback); | ||
| } | ||
| } | ||
|
|
||
| if ((!isIOS() && wasVisible !== isVisible) || (wasVisible && !isVisible)) { | ||
| for (let callback of transitionCallbacks) { | ||
| callback(isKeyboardVisible()); | ||
| transitionCallbacks.delete(callback); | ||
| } | ||
| } | ||
|
|
||
| if (!transitionCallbacks.size && !resizeCallbacks.size) { | ||
| onTransitionEnd(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
In case anyone wonders about why this is poll- instead of event-based:
The decision basically comes down to timing of viewport resize events and updates of the activeElement diffing greatly across platforms and devices, which makes it hard to guarantee execution within an adequate timespan. A couple examples of this include:
- a view resize event landing before a blur/focus event
- activeElement pointing to a stale or transitional element during focus movement
- various interactions may cause OSK closure on different platforms (window focusout, scroll, etc)
In order to flush callbacks within 150ms of the actual event happening - and without having to pass additional element pointers to this function - we need to install a background listener for keyboard resize, and then poll this listener so that updates that have already happened may still be caught.
| &:not([data-open]) { | ||
| opacity: 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm slightly concerned about user land style, when transitions are used for enter animation. Now that entering is delayed by at least a frame, users should be adding something like this to avoid a flash of content. This would be an existing issue in our Popover example styling as well, due opacity not being 0 while awaiting placement, if it were not for the early resize in Popover's layout effect.
I haven't thought of a way to avoid this without forcing an opinionated hidden inline style. Maybe somebody else got ideas?
| status.resizeTimeout = ownerWindow.setTimeout(() => { | ||
| status.isOpen = isKeyboardVisible(); | ||
| delete status.resizeTimeout; | ||
| delete status.resizeTimeStamp; | ||
| }, 150); |
There was a problem hiding this comment.
If someone could assist me with testing on Android, that would be helpful. I only tested this PR on OSX and IOS, across the 3 major browsers. Since we will now reveal overlays when the layout is settled, I want to make sure this is still a good experience.
| (containerDimensions.scroll.top ?? 0); | ||
| // calculate the dimentions of the "boundingRect" which is most restrictive top/bottom of the boundaryRect and the visual view port | ||
| (containerDimensions.scroll.top ?? 0) + | ||
| (visualViewport?.offsetTop ?? 0); |
There was a problem hiding this comment.
@snowystinger This was the core culprit to the positional issue. A scroll into view motion may divide delta between container scroll and viewport scroll, but only the former was accounted for. See this animation for an explainer https://www.w3.org/TR/cssom-view-1/#example-vvanimation
| focus.call(this, {...opts, preventScroll: true}); | ||
|
|
||
| if (!opts || !opts.preventScroll) { | ||
| runAfterKeyboard(() => scrollIntoView(this)); |
There was a problem hiding this comment.
Note this is migrated verbatim, although I believe we don't want to flush immediately on closure of the OSK? Just let me know and we can delay it until the viewport has actually changed.
2 participants
Closes
Before vs After
Screen.Recording.2026-05-23.at.11.11.59.PM.mov
Screen.Recording.2026-05-25.at.6.32.24.PM.2.mov
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: