twbs / bootstrap

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

bootstrap modal enforceFocus() is fundamentally not accessible and not modular / incompatible with Select2 #19971

Closed jzabroski closed 7 years ago

jzabroski commented 8 years ago

Summary bootstrap modal component has a pernicious enforceFocus() helper that interferes with other plugins, such as Select2. There has to be a more accessible way to achieve the goals set out by this helper function than to constantly steal focus.

Proposed Solution I propose bootstrap modal needs a distinct model of the UI in order to handle events in an accessible and modular style, otherwise it will fundamentally be incompatible with other UI components in unpredictable ways.

Observed Behavior When hosting a select2 widget in a bootstrap4 modal, select2 works correctly in IE but does not work in Chrome.

Troubleshooting After digging into the code, it became clear that Chrome was functioning "correctly" and the real problem was in bootstrap modal's use of an enforceFocus() helper. select2 controls inside the modal try to get focus, and the modal outer component steals it away. This is due to the fact select2 is attached directly to the body element [1] , which is the most logical place for third-party controls (e.g. select2) to cache DOM nodes while knowing nothing about the app consuming its behavior. Since select2's DOM nodes are not nested inside the modal but transposed on top via fancy logic, enforceFocus() thinks the user is performing some action outside the modal.

[1] https://github.com/select2/select2/blob/4.0.2/src/js/select2/dropdown/attachBody.js

Test Details:

Example http://jsbin.com/viwizoyofe/2/edit?html,js,output

Example Walk-through Explanation

  1. Go to the Output pane.
  2. Click the dropdown that says red.
  3. Observe that, since bootstrap does not yet see this dropdown as inside a modal dialog, auto-complete inside the control works fine.
  4. Click "Convert to popup", which will trigger bootstrap to see this dropdown as inside a modal dialog.
  5. Click the dropdown that says red, again.
  6. Observe that, since bootstrap sees this dropdown as inside the modal dialog, auto-complete stops working.

When something tries to set focus on anything that is outside of the modal dialog, Bootstrap Modal will return focus to the whole dialog. Incidentally, the dropdown that Select2 creates is outside of the modal dialog (appended to ), including the . When you open the dropdown, Select2 tries to set focus to the , and Bootstrap Modal immediately returns focus to the dialog, leaving focus-less and unable to receive keystrokes.

This autocomplete works before you click the link once, because at that point there is no modal dialog. The element itself exists, but Bootstrap Modal doesn't see it as a modal dialog yet.

This complete example shows that there is nothing wrong with select2 itself, and the problem resides solely in Bootstrap Modal stealing focus. This can be further verified using the following Chrome Debugger steps:

0) Click the "Convert to Popup" link once. 1) Open Dev Tools -> Sourcs -> bootstrap.js -> search for _enforceFocus, set a breakpoint on the innermost statement (for me its line 1910). 2) Click Select2, see the breakpoint hit. 3) Resume execution 4) Click the within the Select2 dropdown, see the breakpoint hit again.

I'm not 100% sure why IE11 allows Select2 to work within Bootstrap Modal, but it could be that IE11 doesn't let you set focus from within a "focus changed" event handler or some such. The dangers of an infinite loop when doing so are even attempted to be handled by Bootstrap Modal, with the following code:

  key: '_enforceFocus',
  value: function _enforceFocus() {
    var _this9 = this;

    $(document).off(Event.FOCUSIN) // guard against infinite focus loop
    .on(Event.FOCUSIN, function (event) {
      if (_this9._element !== event.target && !$(_this9._element).has(event.target).length) {
        _this9._element.focus();
      }
    });
  }

Proposed Solution It would probably be best if we just removed the enforceFocus() helper altogether. There don't seem to be any unit tests that actually test this behavior [1] and it's impossible to the naive consumer to predict which components will work well with it. Even more troubling, IE [11.0.9600.18314 w/ 11.0.31 (KB3154070) update] does not respect enforceFocus() and so it gives the appearance to the client user, comparing IE and Chrome, that there is a "bug" in Chrome.

[1] https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/unit/modal.js

Workaround I ended up just using tether directly and writing my own TypeScript-based PopupViewModel that attaches to document.body. But this is a lot of work to be developing/maintaining my own proprietary popup.

twbs-lmvtfy commented 8 years ago

Hi @jzabroski!

You appear to have posted a live example (http://jsbin.com/viwizoyofe/1/edit), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

You'll need to fix these errors and post a revised example before we can proceed further. Thanks!

(Please note that this is a fully automated comment.)

jzabroski commented 8 years ago

Updated link to add missing rel="stylesheet" on line 7. Problem still reproducible. Cool automation, folks.

cvrebert commented 8 years ago

Erm, we can't remove enforceFocus, at least not without replacing it with something similar. (a11y.js has been suggested, but it's similarly magical/intrusive.) It's what actually makes the modal be modal for non-mouse users. Without it, users could Tab outside of the modal and start interacting with other parts of the page. What I think we need is some better way of allowing users to override the "is this 'within' the modal?" logic and/or a way for users to mark their components as having their own focus management.

There has to be a more accessible way to achieve the goals set out by this helper function than to constantly steal focus.

Such as? Also, you seem to be describing interoperability problems rather than accessibility problems per se.

I propose bootstrap modal needs a distinct model of the UI in order to handle events in an accessible and modular style,

Please explain what you mean by "a distinct model of the UI".

jzabroski commented 8 years ago

Hi @cvrebert ,

Thanks for your thoughtful reply. I am right there with you in all of your thoughts, and tried to guide my explanation carefully to open such a dialog.

Before I respond, let me add that I discovered a feature in Select2 I didn't know existed, but the results are very ugly (I have not figured out why yet). So, Select2 has some interoperability friendliness, but it doesn't display right inside a Bootstrap Modal. Check out my demo here: http://jsbin.com/zivayufebe/edit?html,js,output

Erm, we can't remove enforceFocus, at least not without replacing it with something similar. (a11y.js has been suggested, but it's similarly magical/intrusive.)

Magical/instrusive is in the eye of the beholder. From the perspective of a 3D game programmer who is used to doing hit detection off of a "scene graph" object, the world of Desktop and Web programming is totally bizarre: The idea that the object graph is a recursive ADT and all x,y coordinates are recursively linked for hit detection purposes is just so weird. z-index is literally an after-thought in HTML, something you apply in a css property. Put another way, everyone is "just enough happy" with HTML despite how magical/intrusive it is to assume z-index doesn't exist (most of the time).

It's what actually makes the modal be modal for non-mouse users. Without it, users could Tab outside of the modal and start interacting with other parts of the page.

But I couldn't find a unit test where you actually check that it stays modal in the way you are describing. If I had, it would have failed on IE11 and the Bootstrap code by definition would never pass alpha release for this particular component.

Please explain what you mean by "a distinct model of the UI".

We may be talking past each other a bit. You provided two distinct models of the UI:

What I think we need is some better way of allowing users to override the "is this 'within' the modal?" logic and/or a way for users to mark their components as having their own focus management.

Either may work. I have not done any analysis to know which way is better, but having written a jQuery UI Numeric wrapper for a text box in the past, I know writing "focus management" components is a pain and a business to stay out of entirely if possible.

twbs-lmvtfy commented 8 years ago

Hi @jzabroski!

You appear to have posted a live example (http://jsbin.com/zivayufebe/edit), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:

You'll need to fix these errors and post a revised example before we can proceed further. Thanks!

(Please note that this is a fully automated comment.)

jzabroski commented 8 years ago

@cvrebert Is there a place, before adding an updated example, I can go to check my fixed example against, to minimize noise in this thread?

Anyhow, I have attempted to fix the errors raised by your lmvtfy tool (hopefully it is happy now): http://jsbin.com/zivayufebe/edit?html,js,output

jzabroski commented 8 years ago

One last update. I discovered Select2 created the dropdownParent option strictly because of Bootstrap Modal: https://select2.github.io/options.html#im-using-a-bootstrap-modal-and-i-cant-use-the-search-box

But, there is also further issues (6) already documented with this workaround: https://github.com/select2/select2/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+dropdownParent

In particular, see this issue: https://github.com/select2/select2/issues/4218

I think the last Select2 issue is actually a bug in Tether, related to the fact it does not properly clean up/expose a way to clean up propertly on .destroy() - see https://github.com/HubSpot/tether/issues/36

cervengoc commented 8 years ago

My proposals:

1. Using an explicit option

Introduce an option either on global bootstrap level, or for the modal plugin, which would allow specifying generally which elements should be focusable in any case. The option could be focusables or allowFocus, or so; it's type would be string, and it would represent a jQuery selector which would be checked by the modal's _enforceFocus method like

if ($(event.target).closest(this._config.focusables).length) {
  // ... don't enforce focus
}

2. By trying to be smart

The goal here would be to try to detect if the focused element is kind of a popup.

Modal could detect if the focus is on an element, which is

In this case modal would allow focus on that.


First option is more flexible, but I don't like it too much because maybe introducing a new option shouldn't have such a technical reason. I like the second option, I think it would be satisfactory in most cases, and would solve known problems with select2 and qtip for example.

I'd be interested in implementing any solution for this, as I need this too.

@cvrebert @mdo could you please add your thoughts to make this issue alive?

AlexGnatko commented 7 years ago

So, I had to patch my bootstrap.min.js to turn off enforceFocus. Why not just create an option for modals like data-enforcefocus="true/false", default "true". And if I don't need it - I'd just set it to "false".

pvdlg commented 7 years ago

The option focus exists, and fulfill exactly that purpose. See modal documentation.. By default it's true. If set to false _enforceFocus will never be called for this modal.

AlexGnatko commented 7 years ago

Oh, that's for Bootstrap 4, I'm still using version 3. Never mind ;)

Johann-S commented 7 years ago

Closed thanks to this comment : https://github.com/twbs/bootstrap/issues/19971#issuecomment-275901368

vitorrubio commented 5 years ago

Change dropdownParent to $("#myModal") instead off $("#PopupWrapper") solved my problem: The modal still have "tabindex='-1'" and closes on presing esc. The select2 autocomplete is correctly placed.

`$(document).ready(function() {

$("select").select2({dropdownParent: $("#myModal")});

$('[data-toggle=offcanvas]').click(function() {
  $('.row-offcanvas').toggleClass('active');
});

});`

jzabroski commented 5 years ago

@vitorrubio - Are you saying that you edited my jsfiddle at got it to work? This is a really old issue, and it's even closed actually.... I stopped doing web development 2 years ago.

vitorrubio commented 5 years ago

Yep, I had same problem today: using select2 inside a bootstrap modal. Digging on Google I found your jsfiddle and your code solved my problem (with a litle adjustment). So I posted It in the topic and in a Stack overflow issue.

Thanks, you helped me a Lot.

iamB0rgy commented 1 year ago

None of these works!