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

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

Closed
jzabroski opened this issue May 26, 2016 · 16 comments
Labels

Comments

@jzabroski
Copy link

jzabroski commented May 26, 2016

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:

  • IE 11.0.9600.18314 w/ 11.0.31 (KB3154070) update
  • Chrome 50.0.2661.102 m

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:

  1. Click the "Convert to Popup" link once.
  2. Open Dev Tools -> Sourcs -> bootstrap.js -> search for _enforceFocus, set a breakpoint on the innermost statement (for me its line 1910).
  3. Click Select2, see the breakpoint hit.
  4. Resume execution
  5. 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
Copy link

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:

  • line 7, column 3 thru column 106: Element link is missing one or more of the following attributes: itemprop, property, rel.

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
Copy link
Author

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

@cvrebert
Copy link
Collaborator

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
Copy link
Author

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
Copy link

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:

  • line 27, column 5: E003: Found one or more .rows that were not children of a grid column or descendants of a .container or .container-fluid
  • line 28, column 13: E013: Only columns (.col-*-*) may be children of .rows
  • W002: <head> is missing X-UA-Compatible <meta> tag that disables old IE compatibility modes
  • W003: <head> is missing viewport <meta> tag that enables responsiveness

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
Copy link
Author

@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
Copy link
Author

jzabroski commented May 27, 2016

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: select2/select2#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 shipshapecode/tether#36

@cervengoc
Copy link

cervengoc commented Jul 27, 2016

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

  • not in the modal
  • and visible with normal dimensions (for example larger than 10x10px or so),
  • and has a parent which has position absolute or fixed
  • and that parent has body as its offset parent
  • and even it's z-index could be checked if it's on top of the modal

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
Copy link

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
Copy link
Contributor

pvdlg commented Jan 29, 2017

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
Copy link

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

@Johann-S
Copy link
Member

Closed thanks to this comment : #19971 (comment)

@vitorrubio
Copy link

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
Copy link
Author

jzabroski commented Feb 14, 2019

@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
Copy link

vitorrubio commented Feb 14, 2019

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

This comment was marked as spam.

@twbs twbs locked and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

9 participants