twbs / bootstrap

The most popular HTML, CSS, and JavaScript framework for developing responsive, mobile first projects on the web.
https://getbootstrap.com
MIT License
168.03k stars 78.54k forks source link

Rework modal dialogs #33804

Open patrickhlauke opened 3 years ago

patrickhlauke commented 3 years ago

Current modal implementation appears quirky in current browser/AT combinations, and has some known shortcomings:

We should take closer inspiration from https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html (but that misses the fix for iOS/VO, so would still not be sufficient to copy the approach from there). as mentioned previously (though can't quite find where, worth also looking at approaches like https://github.com/KittyGiraudel/a11y-dialog, and possibly https://github.com/WICG/inert

alpadev commented 3 years ago

You mentioned it there: https://github.com/twbs/bootstrap/issues/28481#issuecomment-757491401.

Not sure if @RyanBerliner continued to work on this, since its seems he was waiting for some acknowledgement on his concept.

Personally I'm totally in for making modals more accessible and I can have a look into it, if that is desired (but I'm also totally fine to leave it up to Ryan as he already started).

#

I just spent some hours reading discussions about this inert proposal and the conclusion seem to be that, after it eventually got dropped, it has been readded to the specs as there is a necessity for smth like that.

I'm just not sure how inert would help us here, that would make it reasonable to add it as a polyfill. Also from what I can tell we tend to dislike to add polyfills to our code.

Please correct me if I'm wrong, but we try to trap the focus into our modal but isn't inert rather doing the opposite as it's purpose is to "remove" a (sub)tree from being accessible e.g. receiving input/focus (beside adding a similiar behavior like aria-hidden="true")?

Wouldn't that mean we have to enforce some certain dom structure or to move a modal into a certain spot in the dom? How is that different than using aria-hidden for that matter? Marking a subtree as non-inert within a tree that was marked as inert already seems not to be in the scope of the current proposal.

As I see it the polyfill currently has to rely on aria-hidden combined with pointer-events and user-select (beside using some JS for handling the focus logic). This behavior also satisfies concerns that got raised, that there are already things in place that partly overlap with the behavior that would be expected from inert.

patrickhlauke commented 3 years ago

Please correct me if I'm wrong, but we try to trap the focus into our modal but isn't inert rather doing the opposite as it's purpose is to "remove" a (sub)tree from being accessible e.g. receiving input/focus (beside adding a similiar behavior like aria-hidden="true")?

yes, the modal itself remains as is, but the underlying page needs to be inert ... so that it's hidden from AT, and its child elements aren't focusable at all. (which also means that generally, you want the modal itself to be a sibling element of the "rest of the page", rather than a child element somewhere in its tree hierarchy)

patrickhlauke commented 3 years ago

As I see it the polyfill currently has to rely on aria-hidden combined with pointer-events and user-select (beside using some JS for handling the focus logic). This behavior also satisfies concerns that got raised, that there are already things in place that partly overlap with the behavior that would be expected from inert.

yes, inert would be the shorthand that makes it much easier in future to do things that currently you have to do as an author with JS (adding tabindex="-1" to everything, adding the pointer-events and user-select, adding aria-hidden="true", etc). the hope is that once natively supported in browser, it'll also be much more performant (particularly if it can avoid the current "walk the entire DOM and sprinkle in tabindex everywhere) approach)

RyanBerliner commented 3 years ago

Yes, I do have a proof of concept changing the current modal focus trap to a more standard, forwards and backwards focus trap. However I haven't continued any work as I was awaiting some confirmation that a change like this would still be welcome in v5. Happy to help again if needed.

That said this doesn't necessarily solve any of the screen reader navigation issues. It seems that aria-modal="true" is the most supported method at this time to trap screen reader navigation. Relative to other methods it's easy to implement and does accomplish its intended goal for quite a lot of users (and bootstrap already uses it in their docs).

Unfortunately the reality is that inert just isn't supported - so I'd think any alternative method or inert polyfill is going to have the do the "walk the dom" dance that you're speaking of avoiding @patrickhlauke. Not only that but they'd need to "walk the dom, and watch the dom" in case things change.

I think a great first, forward moving step would be to fix the tab navigation focus trap. Then some of the other assistive tech bugs may be solvable with tweaks to initial focus and/or the timing of the initial focus.

patrickhlauke commented 3 years ago

It seems that aria-modal="true" is the most supported method at this time to trap screen reader navigation

not supported in VO/iOS (though apparently VO/macOS does respect it)

alpadev commented 3 years ago

@RyanBerliner can't speak for the whole team but IMO it makes sense to continue your work if you're still up for it. As for me, an improved focus trap behavior makes perfect sense in terms of UX alone. If we can use this as a foundation to improve the situation for a11y as well, it's even better.

#

And I found this page https://a11ysupport.io/tech/aria/aria-modal_attribute#support-table-by-assertion-and-at-sr which seems to be a helpful resource for a11y support in general.

alpadev commented 3 years ago

Gonna x-ref https://github.com/twbs/bootstrap/issues/33715 here. Maybe Ryan can solve this too, when he is already improving our modals. 😊 It's somewhat related to focusing.

RyanBerliner commented 3 years ago

Sounds like there's enough interest in improving these dialogs (modal & off canvas bootstrap components) - I should be able to push a PR this week so you all can start testing something tangible.

mkurz commented 2 years ago

With the recent release of Safari 15.4 all major browser now support the <dialog> element natively:

jimmywarting commented 1 year ago

should push for using native <dialog> element instead!

patrickhlauke commented 1 year ago

@jimmywarting @mkurz of course, once the feature has been available for a sufficiently long time and is supported not just in the latest versions of Safari on MacOS/iOS, we will (though likely, as it will be a breaking change, only in a future version of Bootstrap, not a dot release) https://getbootstrap.com/docs/5.2/getting-started/browsers-devices/

patrickhlauke commented 1 year ago

as mentioned in #37000 switching - for v6 - to using <dialog> will resolve a lot of related issues here caused by screen reader shortcomings and our overly complex legacy implementation.

suchislife801 commented 9 months ago

\<dialog>: The Dialog element

Reference Feature Request: #39191

Implement native <dialog> element in Bootstrap for enhanced accessibility and usability.

Proposal to Implement Native <dialog> Element in Bootstrap

MDN - The dialog element

Introduction

The native HTML <dialog> element has recently gained full browser support. It offers built-in accessibility and usability features, making it an ideal choice for modal dialogs. This proposal suggests that Bootstrap should adopt the <dialog> element to enhance its dialog components.

Benefits

Example Code

<dialog id="myDialog">
  <p>Dialog content here</p>
  <button id="closeBtn">Close</button>
</dialog>

Built-in Methods for <dialog>

.show()

.showModal()

.close([returnValue])

Conclusion

Incorporating these built-in methods will enhance Bootstrap's dialog components in terms of usability and accessibility.

A playground Example

JsFiddle Example

Example using existing CSS Implementation

<!doctype html>
<html lang="en" data-bs-theme="dark">
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Bootstrap Dialog demo</title>
    <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.2/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-T3c6CoIi6uLrA9TneNEoa7RxnatzjcDSCmG1MXxSR1GAsXEV/Dwwykc2MPK8M2HN" crossorigin="anonymous">
  </head>
  <body>

    <h1 class="text-center">&lt;dialog&gt;: The Dialog element</h1>

    <main class="container" style="height: 100px;">

      <section class="row">
        <div class="d-flex justify-content-center align-content-center">
          <!-- Button trigger dialog -->
          <button id="launchDialog" type="button" class="btn btn-primary">
            Launch demo dialog
          </button>
        </div>
      </section>

      <!-- Dialog -->
      <dialog id="exampleDialog">
        <div class="dialog-content">
          <div class="dialog-header">
            <h1 class="dialog-title fs-5">Dialog title</h1>
            <button id="closeDialog" type="button" class="btn-close" aria-label="Close"></button>
          </div>
          <div class="dialog-body">
            Text in the dialog body...
          </div>
          <div class="dialog-footer">
            <button type="button" class="btn btn-secondary" id="closeDialogBtn">Close</button>
            <button type="button" class="btn btn-primary" id="saveDialogBtn">Save changes</button>
          </div>
        </div>
      </dialog>

    </main>

    <script type="text/javascript">

      // References
      const launchDialog = document.getElementById("launchDialog");
      const exampleDialog = document.getElementById("exampleDialog");
      const closeDialog = document.getElementById("closeDialog");
      const closeDialogBtn = document.getElementById("closeDialogBtn");
      const saveDialogBtn = document.getElementById("saveDialogBtn");

      // Event - Open the dialog
      launchDialog.addEventListener("click", () => {
        exampleDialog.showModal();
      });

      // Event - Close the dialog
      closeDialog.addEventListener("click", () => {
        exampleDialog.close();
      });

      // Event - Close [X] the dialog
      closeDialogBtn.addEventListener("click", () => {
        exampleDialog.close();
      });

      // Event - Save & Close the dialog
      saveDialogBtn.addEventListener("click", () => {
        console.log("Save something...");
        exampleDialog.close();
      });

    </script>

  </body>
</html>
patrickhlauke commented 9 months ago

as before, need to be cogniscent of https://getbootstrap.com/docs/5.3/getting-started/browsers-devices/

suchislife801 commented 9 months ago

HTML \<dialog> element - Browser Compatibility For Reference

mkurz commented 9 months ago

as before, need to be cogniscent of https://getbootstrap.com/docs/5.3/getting-started/browsers-devices/

I think browser native modal is for bootstrap 6 and should definitely be considered for including.

rijenkii commented 2 weeks ago

For <dialog>, be mindful of the fact that in Firefox ::backdrop still cannot be animated: https://bugzilla.mozilla.org/show_bug.cgi?id=1725177.