Skip to content
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

Re-add nested portals #2297

Merged
merged 12 commits into from
Oct 17, 2024
Merged

Re-add nested portals #2297

merged 12 commits into from
Oct 17, 2024

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Oct 10, 2024

Changes

Reverts #2209 as the fix for #2205 (comment). Did this revert after internal discussion after we realized that a simpler alternative fix also works.

The simpler alternative to #2209 is to move focus to the DatePicker's today's date (second popover) only after it has been portaled to be a descendant of DateRangeFilter. Since floating-ui's FloatingFocusManager uses DOM events instead of React events (floating-ui/floating-ui#3060), waiting for the second popover to be portaled within the first is important to prevent a focus out being triggered on the first popover.

Additionally, I was unable to figure out how #2209 fixed the bug. Because after #2209, the second popover was moved outside the DOM of the first popover. So, focusing the second popover should be treated as a focus out of the first popover prompting the first popover to close. But it didn't. Since we're unable to figure out why it happens, reverting is safer as it brings a more reliable and predictable state.

Testing

  • Fix failing unit tests
  • Approved failing Table test images. Some of them may be reverts of the test images approved in #2209.
    • Even though the new images show that focus does not move to the correct element in the floating content (even with cy.wait()), I manually confirmed that focus moves correctly to the expected element in the floating content.
    • Additionally, the images with the incorrect focus were approved before #2209. So, it's okay to re-approve these images that don't have the focus on the correct element?

Docs

Not sure if a changeset is needed since I don't think it's a significant user facing change.

After PR TODO:

  • Could consider a general "overridable initial focus" mechanism. Would be useful in Dialog too. (#2297)

@r100-stack r100-stack self-assigned this Oct 10, 2024
@r100-stack r100-stack changed the title DatePicker self focus *after* portaling Re-add nested portals Oct 11, 2024
@r100-stack r100-stack marked this pull request as ready for review October 11, 2024 15:46
@r100-stack r100-stack requested a review from a team as a code owner October 11, 2024 15:46
@r100-stack r100-stack requested review from mayank99 and smmr-dn and removed request for a team October 11, 2024 15:46
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

This needs a changeset because it has a pretty noticeable impact for users. And doesn't it also fix #2245?

@r100-stack
Copy link
Member Author

r100-stack commented Oct 16, 2024

This needs a changeset because it has a pretty noticeable impact for users.

Added the changeset nearly identical to the one from #2209. Can make it more specific if needed. Didn't think it was needed since the exact details of the fix in #2209 weren't added in its changeset.

And doesn't it also fix #2245?

Yes, it fixes #2245 and #2291. I tested it by confirming that the nested popovers in the playgrounds' code no longer close unexpectedly.

Screen-recordings
PR Before After
#2245
before.mov
after.mov
#2291
before.mov
after.mov

Copy link
Contributor

@smmr-dn smmr-dn left a comment

Choose a reason for hiding this comment

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

LGTM!

@jason-crow
Copy link

@mayank99 when can we expect this PR to merge in and a patch published including it? This is needed to resolve a blocker for our pending release and it seems like the PR is approved, so I'm hoping this can complete and publish today?

@r100-stack r100-stack enabled auto-merge (squash) October 17, 2024 13:43
@r100-stack r100-stack merged commit 7cd1976 into main Oct 17, 2024
18 checks passed
@r100-stack r100-stack deleted the r/focus-after-portaling branch October 17, 2024 13:50
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 17, 2024
@r100-stack
Copy link
Member Author

when can we expect this PR to merge in and a patch published including it?

This patch fix has now been released in 3.15.4 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression with nested popovers Combobox / Select inside a Popover
4 participants