feat: support for interactive widgets in overlay placement - #10102
feat: support for interactive widgets in overlay placement#10102nwidynski wants to merge 48 commits into
Conversation
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 isKeyboardVisible.ts and runAfterKeyboard.ts. I will see whether I can also add tests once review is in 👍 |
nwidynski
changed the title
| 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.
|
@snowystinger Sorry to bump, but is there any way this makes the cut for the next release? Ive got the rework of resize observer ready and would love to include this PR without inflating the diff. Is there any major concern left here? |
|
@nwidynski sorry, I saw the red "x" and haven't returned to review. I'll move it up my personal todos, but it's unlikely to make it in this release as we're aiming for early next week. |
# Conflicts: # packages/react-aria/src/utils/useViewportSize.ts
|
Hope you don't mind, so that it's hopefully green next chance I get and ready, I merged main and fixed the conflict and fixed the typecheck. Please check that i resolved it correctly |
|
Not at all, thanks for doing that actually! I think you will want to revert the tsconfig(s) also if you want to remove the |
There was a problem hiding this comment.
Manual testing worked really well for me on MacOS and iOS. It's a big improvement for me, the url bubble never covers the overlays anymore.
There is an odd case on Android, it doesn't always happen. If I'm already focused on the input with the keyboard up but the popover not open in S2 ComboBox, then there can be a significant delay to opening the overlay. I've been unable to come up with any reason for it. Otherwise it's working really well on Android too.
Will need to spend some more time in the code. Looks like it's already marked ready for review, so hopefully can get another set of eyes on it in the near future. I am still a little apprehensive about the isOpen state and associated style.
|
|
||
| // Update the screen once an open keyboard that rotated along closes. The state swap was | ||
| // deferred, so the old width predicts the height it should close towards. | ||
| if (Math.abs(rotation) % 180 && state.screenWidth - screen.height < KEYBOARD_HEIGHT) { |
There was a problem hiding this comment.
for anyone else unfamiliar with screen.angle, it can only be 0, 90, 180, or 270
|
@snowystinger Great to hear! I agree it feels much better, especially for Modals, since now the centering of the input doesnt abort as it used to. There's also plenty we can do with this in terms of additional QOL improvements; Especially the Autocomplete experience on mobile has been bugging me. Regarding the odd behavior on Android, are you able to reproduce this multiple times in sequence? Meaning not in separate attempts but throughout subsequent open and closings? The keyboard detection logic should auto-adjust in the background meaning if it fails we at most have one delayed opening, with the next opening being fast again. If thats not the case, then this issue likely pre-existed, but again, hard for me to test. |
Yeah, my thought was initial open, but it was happening multiple times in a row for me, and then it would stop, and then start again. I'll check tomorrow to see if I can reproduce it on main as pre-existing. |
# Conflicts: # packages/react-aria/src/utils/domHelpers.ts
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Alright, I think I found the issue with android. It appears to only happen with the combobox because the keyboard is staying open between clicks.
- Go to docs
/s2/ComboBox - tap combobox menu trigger to open combobox
- tap combobox menu trigger to close combobox
- tap combobox menu trigger to open combobox <- delayed on Android
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
provisional approval based on behaviour improvements, but I'd like to signal more people to review. there's still a lot of code to think through.
|
|
||
| function onResizeEnd(): void { | ||
| // Overlaying keyboards do not impact geometry, so there is nothing to measure. | ||
| if (getMetaValue('viewport')?.includes('overlays-content')) { |
There was a problem hiding this comment.
Hey, got any ideas on how to test this one? Do some keyboards just add this automatically?
@LFDanLu noticed it's not supported in iOS. Is that because there are no overlaying keyboards?
There was a problem hiding this comment.
Synthetic or manual? There is a synthetic test included in the test file iirc. For manual testing you will probably want to check on Android and have Claude scaffold a testbench, since that's where its fully supported.
FF on iOS just so happened to be an example of this, so we no longer need to explicitly check for it
I suppose this statement lead to confusion, so let me clarify: What I meant was that Firefox on iOS behaves as if it were Android and the meta tag were set to resizes-content, so proper support for Android fixed it naturally.
I don't believe this is done on purpose through setting a meta tag in the background, but I also haven't checked. As far as I know, it was just a bug in the Firefox iOS implementation, and UAs generally don't set this tag automatically. It's pretty much left to each user implementation to opt-out of the default behavior, which the Chromium team mentioned to have aligned with iOS on purpose.
That said, we should skip this check on iOS entirely for as long as its unsupported to avoid false positives.
# Conflicts: # packages/react-aria/src/overlays/usePreventScroll.ts # packages/react-aria/src/utils/getMetaValue.ts
3 participants
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: