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

Keep role="dialog" after closing modal/offcanvas if already in the markup #39941

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Apr 30, 2024

Description

This PR is the result of a discussion that happened in https://github.com/orgs/twbs/discussions/39933.

TL;DR: if a role="dialog" is in the modal/offcanvas markup, our JavaScript will remove it when closing the element. The idea of this PR is to integrate a modification or our JavaScript behavior to keep this role if it is present in the markup.

There was a scenario to handle that, which was to put back the role="dialog" in the markup and doing nothing in the JavaScript by expecting users to correctly use it. But I like the enforcement rule that is present right as a modal/offcanvas can't be used without JavaScript right now anyway.

So I preferred the approach in this PR that doesn't change anything in the current behavior, but allows some flexibility for folks still wanting to add the role="dialog" in the markup for any reason.

Please note that the documentation is not modified as it's still accurate (source: https://getbootstrap.com/docs/5.3/components/modal/#accessibility):

Note that you don’t need to add role="dialog" since we already add it via JavaScript.

First, dear reviewers, what do you think of this approach, in terms of JavaScript and/or accessibility? As mentioned in the GH discussion, I deducted information in the history, but maybe it's wrong, and you have more history to share.

If we agree to go this way, is handling only role="dialog" this way enough, or do you think we also should do the same thing for aria-modal which follows the same logic?

While

Type of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • (N/A) My change introduces changes to the documentation
  • (N/A) I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related GH discussion

@patrickhlauke
Copy link
Member

while i don't think it's necessarily needed, i can see how the idea of "our code cleans up after itself" works here.

I would say it may actually be good/make sense for our documentation to say/show that you should ideally add role="dialog" directly in your markup (but that our JS error-corrects for this in case it isn't there)

@julien-deramond
Copy link
Member Author

I would say it may actually be good/make sense for our documentation to say/show that you should ideally add role="dialog" directly in your markup (but that our JS error-corrects for this in case it isn't there)

So it would mean:

  1. Keeping my JS modification in this PR
  2. Kind of reverting the modifications done in the HTML from https://github.com/twbs/bootstrap/pull/30936/files#diff-78bcedc866a59b217f5fb00fb270b86c4acc3bb0ff01a712cf0b8754715bee49 (+ other files) by re-adding everywhere role="dialog" for modals and offcanvases.
  3. Slightly improving the doc to explain that there's a safety net in our JavaScript that will add the role by default.

TBH, I prefer this approach where the DOM is correct right away, and the JS tries to help when it can. I hadn't suggested this approach because #30936 explicitly did the opposite. But if you're OK, and the rest of the team too, I can modify this PR in this direction.

@patrickhlauke
Copy link
Member

I think the one problem we had with having the role="dialog" directly in the documentation was that, for modals that we want to show already open (like the "modal components" example one), that will cause issues. If we can somehow not have that there in the rendered version, that'd then work

@julien-deramond
Copy link
Member Author

Oh, I see! OK then, I won't have time to do it right away, but I'll try to apply everything we said in this PR. So I let it in draft until I have time to do it later on. Thanks a lot for the early feedback, Patrick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants