fix: accessibility issue modal

[shrilakshmishastry]

Description

Accessibility issue in Modal

Ally pattern of modal: Modal is a window overlaid on either the primary window or another dialog window. Windows under a modal dialog is inert. That is, users cannot interact with content outside an active dialog window. Inert content outside an active dialog is typically visually obscured or dimmed so it is difficult to discern, and in some implementations, attempts to interact with the inert content cause the dialog to close.

read more

Issue: When the modal is open windows under a modal dialog are accessible by the keyboard.

Link to issue description

Fix: Use dialog tag and call showModal() method on the mount of Modal component

Screen recording

then	now
https://github.com/usebruno/bruno/assets/29778698/20ac9709-7967-4ccb-a580-578fd9ece1fc	https://github.com/usebruno/bruno/assets/29778698/00c0b4e0-c5f9-4889-8c4c-9f3001ed946d

Contribution Checklist:

• [x] The pull request only addresses one issue or adds one feature.
• [] The pull request does not introduce any breaking changes
• [x] I have added screenshots or gifs to help explain the change if applicable.
• [x] I have read the contribution guidelines.
• [x] Create an issue and link to the pull request.

[PushpenderSaini0]

Can you remove the package-lock.json changes from this PR !

[shrilakshmishastry]

Can you remove the package-lock.json changes from this PR !

I tried here but it didn't get updated 😬

[PushpenderSaini0]

Can you remove the package-lock.json changes from this PR !

I tried here but it didn't get updated 😬

The file is changed in 2 commits this one will be a bit tricky !

1 ) Checkout the file from the main branch 2) Commit the file (this version is from main branch) 3) (Optional) Squash the changes to make the history clean !

git checkout main -- package-lock.json
git commit -a -m "revert package-lock.json"
git rebase -i HEAD~4

[shrilakshmishastry]

Hi @shrilakshmishastry as far as understand the issue (from description and screen recordings) is that focus is going out of the active modal/dialog when navigating through the keyboard and hence you are trying to make sure that the focus stays in active dialog. Please let me know if my understanding is correct.

However the solution of triggering showModal when mounted isn't clear to me and how is it fixing this issue ? We should instead have a keydown handler for modal component which makes sure that on keyboard navigation the focus always keeps looping (in order) inside the active dialog unless esc is pressed ? A small suggestion regarding the screen recording - you can directly upload the .mov files so reviewers don't have to download the files to view the screen-recordings :)

For reference - I came across this PR in https://twitter.com/shrilakshmihg/status/1779420061721035153

Hey @ad1992 thanks for the suggestion!!

yes, the focus is going out of the modal, hence the fix.

calling showModal in useEffect is to set dialog open. It's a way around not using JS to handle focus the one you mentioned throw key-down event.

The intention of using the dialog tag is its ability to handle focus traps in an HTML declarative way not by JS.

you can read more about it here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

The doc says you need to call showModal method in the caller component. Since we are handling showing Modal via js (mounting it to DOM) on button click there is no straight way we can access dialog. Hence calling showModal in useEffect so dialog provides the expected behavior when it's in open state and act as Modal.

[shrilakshmishastry]

suggested a minor change otherwise looks good @shrilakshmishastry

I am not sure if there is a way to test the changes directly in the browser so I haven't tested it however the code looks good!

Additionally, I think you probably might be targeting the rest of the clean-up in separate PR (eg closing the modal via dialog close and removing ESC handler etc)

closing modal is taken care in closeModal() method and esc handling is already present in the existing code

[ad1992]

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

@shrilakshmishastry as per the docs, I think you can remove the ESC handling as it shouldn't be needed anymore and taken care of by the dialog itself.

[shrilakshmishastry]

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

@shrilakshmishastry as per the docs, I think you can remove the ESC handling as it shouldn't be needed anymore and taken care of by the dialog itself.

We need it, as there are cases where we needn't close the modal on the click of esc key. There is one prop the modal accepts and based on that action on the esc key-down is taken care

https://github.com/usebruno/bruno/blob/d490b1b5fc2cea0d8c0aa60d4ab07210f4d647cc/packages/bruno-app/src/components/Modal/index.js#L67C2-L67C20

[shrilakshmishastry]

I found that the "toasts" are now in the background, behind the dialog/backdrop instead of the foreground. This is probably due to how the dialog element works: https://stackoverflow.com/a/41816764

Seems intersting, I will look into this. Thanks

[shrilakshmishastry]

I found that the "toasts" are now in the background, behind the dialog/backdrop instead of the foreground. This is probably due to how the dialog element works: https://stackoverflow.com/a/41816764

Seems intersting, I will look into this. Thanks

The toast needs to be attached to or decendent of top layer element when triggered from them. I have rised the issue in react-hot-toast. Not sure whether it should be fixed from them or here. I will look into this furthur https://github.com/timolins/react-hot-toast/issues/355

[helloanoop]

@anusreesubash Please review this?