New EnforceFocus on overlay causes dialogs to jump

[ro-savage]

Environment

• Package version(s):
  "@blueprintjs/core": "^3.50.2",
  "@blueprintjs/datetime": "^3.23.12",
  "@blueprintjs/popover2": "^0.12.2",
  "@blueprintjs/select": "^3.18.4",
• Operating System: MacOS
• Browser name and version: Chrome 93

Code Sandbox

https://codesandbox.io/s/blueprint-sandbox-forked-5m8ok?file=/src/index.tsx

Steps to reproduce

Open dialog, scroll to the date picker and select a date. The user is thrown back up to the top of the screen (the first thing has can be focused)

Actual behavior

The user is thrown back to the first focusable element

Expected behavior

The user should remain at the current position

Possible solution

I believe this was introduced by #4894

When you click the date picker, the dialog loses focus, then when you select a date, the dialog regains focus and the new change now automatically focuses the first element.

I would expect this only to apply when a overlay/dialog/etc was first opened, not after it's already been open.

[ro-savage]

@michael-yx-wu - I believe shouldReturnFocusOnClose should also be added to DatePickers and any other similar elements.

[michael-yx-wu]

I think you're right. Seems like it shouldReturnFocusOnClose should default to false for DatePickers to avoid re-opening the DatePicker after a date has been selected.

[adidahiya]

@ro-savage I'm not sure how to use your code sandbox... the dialog doesn't scroll or jump...?

Also, #4919 disabled shouldReturnFocusOnClose for all popovers, which includes DateInput. So I believe this should be resolved already?

[michael-yx-wu]

I think he meant to enclose the DateInput inside the dialog instead of making it a sibling.

[adidahiya]

Can someone post an updated sandbox demonstrating the bug?

[ro-savage]

@adidahiya - Sorry it looks like the sandbox didn't actually save my changes last time.

I've just re-created it at the same link.
https://codesandbox.io/s/blueprint-sandbox-forked-5m8ok?file=/src/index.tsx

Here is a video of the issue as well
https://www.loom.com/share/c601519f6dc54c939ad817fb723d59c6

[adidahiya]

@ro-savage ok, thanks for the updated sandbox and the video. I can clearly see the issue now.

You can easily work around this issue by setting <Dialog enforceFocus={false}> on your dialogs where you expect to have a scrollable body container. Most dialogs don't scroll, so I feel that it's probably ok for the component default to remain enforceFocus={true}, and users like you can disable it on a case-by-case basis if this behavior causes issues with focus management.

Maybe there's something more clever we can do here in Overlay during the enforceFocus implementation... perhaps we should avoid focussing the firstKeyboardFocusableElement if it's not visible in the current scroll view (see screenshot below)? I'd be open to a PR which implements this kind of change, but it's no longer P1 priority for me since there is a workaround.