fix: overlay layout shift when auto-focusing input#10102
Conversation
| preventScrollCount++; | ||
| if (preventScrollCount === 1) { | ||
| if (isIOS()) { | ||
| if (isIOS() && isWebKit()) { |
There was a problem hiding this comment.
Regressed in #1514. Not sure if lucky accident though? Will need testing.
There was a problem hiding this comment.
So to confirm, the changed flow is now that we need to test iOS Chrome and iOS Firefox as they'd be the two which used to go through here and no longer do?
There was a problem hiding this comment.
Correct. I did that, and havent found any regressions, but I would feel better with an additional test session.
Note I did find a pre-existing bug that window scroll is possible when pinch zooming outwards (some browser must first be pinched inwards to then pinch outwards) and down, aka two finger scrolling. Outwards pinch in general is pretty funky - even on Safari Desktop.
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 isKeyboardVisible.ts and runAfterKeyboard.ts. I will see whether I can also add tests once review is in 👍 |
| 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.
| } else if (isShadowRoot(currentNode)) { | ||
| // Element is in shadow root | ||
| currentNode = currentNode.host; | ||
| if (isHTMLElement(current) && !isSlotElement(current) && current.assignedSlot?.parentNode) { |
There was a problem hiding this comment.
Note that changes to DOMFunctions.ts and domHelpers.ts are largely unnecessary for the purpose of this PR, but are included due to this PR being extracted from a larger set of changes, for which they are necessary and useful.
PS: @snowystinger FYI, this line specifically did not match the original tabster source so I fixed that.
There was a problem hiding this comment.
Thanks, if they are unnecessary for this PR, I'd love to pull them out into their own PR with tests that show why they are necessary. Happy to assist with it, I'll have some time this coming week.
What's the original tabster source? I've forgotten what that is a reference to.
There was a problem hiding this comment.
See license notice at the top of the file. Feel free to rip them out, otherwise I will probably get to it over the weekend.
Address PR review feedback. Replace the scroll-specific addGlobalScrollListener with a generic getShadowRoots(node) helper plus the addEvent utility, since the shadow-DOM event-propagation problem applies to all non-composed events, not just scroll. addEvent and the EventMapType/EventTargetType type helpers are pulled in from adobe#10102 verbatim and placed at the same locations so the overlap auto-resolves when adobe#10102 lands. Callers (ScrollView, both useCloseOnScroll copies) now use: addEvent(window/document, 'scroll', listener, true) addEvent(getShadowRoots(ref.current), 'scroll', listener, true) Select.browser.test now opens the ComboBox via the @react-aria/test-utils combobox tester instead of dispatching raw PointerEvents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…per caller Collapse the per-caller two-addEvent-calls pattern to one. getShadowRoots is replaced by getEventTargets(global, refNode), which returns [global, ...shadowRoots] as a single EventTarget[] (addEvent already accepts arrays and returns one combined cleanup). getEventTargets only collects the shadow roots that lie strictly between refNode and global, stopping at global's own root node so it works correctly even when a non-window/document node is passed as global. addEvent's body stays byte-for-byte identical to adobe#10102; only its JSDoc gains a note about composed:false events (scroll, scrollend) and when to use getEventTargets, so the implementation still auto-merges with adobe#10102. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # packages/react-aria-components/src/Modal.tsx # packages/react-aria-components/src/Popover.tsx # packages/react-aria/src/selection/useSelectableCollection.ts
snowystinger
left a comment
There was a problem hiding this comment.
Thanks for the patience.
Testing on Chrome MacOS desktop, running yarn start:s2-docs locally. If I go to http://localhost:1234/s2/Dialog then click to open a Modal, it flashes before opening.
I've checked, it's doing it in FF, Safari, and iOS Safari as well. I've also checked, it's not pre-existing on Main.
dialogflicker.mov
|
|
||
| export interface ModalRenderProps { | ||
| /** | ||
| * Whether the modal is ready to be displayed. Use this to avoid layout shift. |
There was a problem hiding this comment.
Doc is a little particular to the issue you're solving, but not really a great example of when to use without the knowledge of this PR and Issue. Have any ideas how we can improve this?
There was a problem hiding this comment.
Suggestions welcome 😅 Maybe something like ”[…] Use this to hide the modal while it is not yet ready to be placed."
| @@ -1 +1,6 @@ | |||
| export {isCtrlKeyPressed, willOpenKeyboard} from '../../../src/utils/keyboard'; | |||
| export { | |||
There was a problem hiding this comment.
why the double up on this file? they appear to be the same and are adding to noise in this large PR
can we just keep the old one? should cut down on some more noise and prevent a new private subpath
There was a problem hiding this comment.
No strong reason. I mostly just wanted to rename source file to isKeyboardVisible to align with other file naming in utils. I left the keyboard export file as is and pointed at the new source file to not break anything, but if you dont want to rename we can also just keep the file name as it was or otherwise defer into a chore followup.
| }, [ref, state]); | ||
|
|
||
| // @ts-ignore https://github.com/facebook/react/pull/24741 | ||
| // https://github.com/facebook/react/pull/24741 |
| } else if (isShadowRoot(currentNode)) { | ||
| // Element is in shadow root | ||
| currentNode = currentNode.host; | ||
| if (isHTMLElement(current) && !isSlotElement(current) && current.assignedSlot?.parentNode) { |
There was a problem hiding this comment.
Thanks, if they are unnecessary for this PR, I'd love to pull them out into their own PR with tests that show why they are necessary. Happy to assist with it, I'll have some time this coming week.
What's the original tabster source? I've forgotten what that is a reference to.
| } | ||
|
|
||
| /** | ||
| * Type guard that checks if a value is an Element. Uses window self reference checks to |
There was a problem hiding this comment.
| * Type guard that checks if a value is an Element. Uses window self reference checks to | |
| * Type guard that checks if a value is a Window. Uses window self reference checks to |
|
|
||
| /** | ||
| * Delays a callback execution until a keyboard transition may no longer impact layout. | ||
| * Guarantees an invocation if an expected transition did not finish within 300ms. |
There was a problem hiding this comment.
300 or 600? it doesn't look like there's a split path between the two from here
There was a problem hiding this comment.
Both timeout after 300ms, except for runAfterKeyboardTransition on iOS close, which needs to wait 600ms because the single viewport resize will land when fully done. I just documented the worst case for each one.
Split path is in transitionTimer. On iOS open we purposefully let the timeout hit to flush transition callbacks.
| preventScrollCount++; | ||
| if (preventScrollCount === 1) { | ||
| if (isIOS()) { | ||
| if (isIOS() && isWebKit()) { |
There was a problem hiding this comment.
So to confirm, the changed flow is now that we need to test iOS Chrome and iOS Firefox as they'd be the two which used to go through here and no longer do?
| // Wait one frame to see if focus lands on an input. | ||
| let frame = ownerWindow.requestAnimationFrame(() => { | ||
| let activeElement = getActiveElement(ownerDocument); | ||
| let willKeyboardOpen = ownerDocument.hasFocus() && willOpenKeyboard(activeElement); |
There was a problem hiding this comment.
won't this delay modals on desktop by the maximum (300) as well?
There was a problem hiding this comment.
Unfortunately yes, but that was deliberate. We have no way of knowing whether a device supports an OSK, and there are desktop devices with one, e.g. 2in1 laptops.
Personally I would be fine with limiting this to iOS and Android, but that may be a thing to discuss with the team and also test for.
An idea would be to store whether OSK has revealed at some point globally, to then skip or not skip this thing. This way we would only run into the timeout on first open, and only if no input has previously been focused.
Quite frankly, most of this PR is really disappointing to need in the first place, but it hopefully will become obsolete once VirtualKeyboard API is widely available.
|
|
||
| let transitionTimer = isIOS() && isWebKit() && wasOpen ? 600 : 300; | ||
|
|
||
| if (transitionInterval != null && transitionTimeout != null) { |
There was a problem hiding this comment.
| if (transitionInterval != null && transitionTimeout != null) { | |
| if (transitionInterval != null || transitionTimeout != null) { |
should it be this?
There was a problem hiding this comment.
Doesnt matter, but sure. These only move together.
| function scrollIntoView(target: Element) { | ||
| let root = document.scrollingElement || document.documentElement; | ||
| let nextTarget: Element | null = target; | ||
| while (nextTarget && nextTarget !== root) { |
There was a problem hiding this comment.
we should check that nextTarget isConnected, we're running scrollIntoView possibly 300+ ms after a blur now, so probably a good check to have
There was a problem hiding this comment.
Sure, although I'm hoping to rip this whole thing out after the rework to scrollIntoView is finally done.
|
@snowystinger I assume S2 Dialog uses transitions for enter animation? If thats the case then I likely just missed adjusting the stylesheet. This is exactly the concern I had mentioned in #10102 (comment) Will check later today when I'm in the office, but generally speaking all s2 components utilizing Popover or Modal internally should be tested for these flashes. |

Closes layout shift due to OSK open or close resizing the visual viewport prior to overlay placement.
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: