search

primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.19k stars 538 forks source link

Dialog: Focus shifts unexpectedly on opening #4363

Closed maximedegreve closed 2 months ago

maximedegreve commented 8 months ago

Description

I've made a Dialog that has 4 items you can choose from (ActionList items). When I open the dialog, it automatically highlights the last item you can choose.

There are multiple problems happening here.

https://github.com/primer/react/assets/980622/f750b27c-8c1e-4e5d-925d-5f644d156f4e

Steps to reproduce

  1. Internally go to our Primer Recipes example
  2. Click Assigned

Version

Latest

Browser

Safari

siddharthkp commented 8 months ago

@maximedegreve Tried to reproduce and couldn't 🤔. Tested on Safari and Chrome

Are there more steps required?

https://github.com/primer/react/assets/1863771/f66f2dcc-a5c9-4171-a62b-afc54bdffb46

maximedegreve commented 8 months ago

@siddharthkp I'm not sure why this works differently on mine here.

I've took a full window video to demonstrate my setup.

https://github.com/primer/react/assets/980622/63c5e7b4-98ee-4378-85ae-6a3d7e77c602

siddharthkp commented 3 months ago

Figured out how to reproduce this:

MacOS → Settings → Appearance → Show scroll bars → Always

MacOS settings with always show scroll bars selected

Why does it happen:

Not sure what's the move here. cc @mperrotti any ideas?

dipree commented 3 months ago

@siddharthkp this is likely related, I believe it wasn't like this before we switched the component here recently 🤔:

https://github.com/user-attachments/assets/98772be2-28bf-42f1-938b-85f0832dc671

siddharthkp commented 3 months ago

Yep, same component. The change isn't new (July 2023) but as the Dialog gets more usage, this keeps popping up in more places

If we remove this behavior, you end up background scrolling, see video:

https://github.com/user-attachments/assets/653f814e-1a33-46aa-8bdf-ce12dc04def5

Notes:

  1. top-layer / popover does not solve for this
  2. overscroll-behavior: contain would help but only if the dialog has overflow not if the dialog does not have overflow like in the sidebar example (browser bug report: chrome, safari)

@keithamus @mperrotti Can I borrow your minds here?

keithamus commented 3 months ago

Looks like --dialog-scrollgutter isn't being applied which is causing the shift in the underlying body content...

But that's not the issue @maximedegreve is describing here, is it? It looks like OP describes the focus of the dialog starting on the 4th element. Native <dialog> does solve for this; it'll focus the first focusable item, or the item marked with the autofocus attribute. I presume there's a bug in our focus trap code which our React dialog is using.

maximedegreve commented 3 months ago

@keithamus is correct. This is not about the scrollbars here.

siddharthkp commented 2 months ago

Oh, my bad. I may have mixed a different issue with this one and went on the wrong track

I tried reproducing the focus jumping and could not reproduce it in Safari or Chrome 😔

I presume there's a bug in our focus trap code which our React dialog is using.

That's possible, @keithamus were you able to reproduce it as well?

siddharthkp commented 2 months ago

Confirmed with @maximedegreve, we can't reproduce it anymore. My guess is that this was a bug in the recipes component that has now been fixed. Can reopen if we see this or a similar bug again