Text selection within trapped element not possible if any ancestor has a `tabindex` about focus-trap HOT 9 CLOSED

I don't understand why tryFocus is called here instead of only preventing the focusin event from propagating like the handlers for other types of events. I think removing that line will fix the issue, but I'm not sure if it causes other problems.

from focus-trap.

I don't understand why tryFocus is called here instead of only preventing the focusin event from propagating like the handlers for other types of events. I think removing that line will fix the issue, but I'm not sure if it causes other problems.

@mkantor Thank you for your analysis and for the demo. The tryFocus() is there because, so far in the implementation, we believe focus has escaped the trap, so we need to pull focus back into the trap, and to do that, we have to try to focus something inside the trap (i.e. the most recently focused node, or the initial focus node).

So I can see that no matter the tabindex value (-1, 0, 1) of the trap's ancestor, text selection doesn't work. I'm guessing that's because making the div focusable means clicking anywhere inside it -- other than directly on a focusable child -- means it either triggers a click on the div, and because the div is not inside the trap, focus-trap determines focus has escaped and pulls focus back in with the tryFocus() in the else case.

Or, it triggers a click on the selectable text, but since it's not tabbable, it never gets registered in the trap's containers, which we first search and so again, we think focus has escaped and we need to pull it back in.

Given it's possible to select the text inside the trap when the ancestor div is not tabbable nor focusable (i.e. does not have a tabindex attribute), I'm guessing it's the first case: Click on non-tabbable/focusable text inside the ancestor div triggers click on the ancestor div (even with tabindex="-1", it's possible to set focus to it by clicking on it), leading focus-trap to detect trap escape, pulling focus back in.

That seems considerably harder to fix than if it were just the text not being in the trap's containers, which doesn't seem to be an issue when the ancestor div isn't tabbable/focusable since boilerplate text is never either. Simply selecting the text does not (I checked) trigger any change to the focus (doesn't trigger focusIn event, anyway), even though something that had focus inside the trap will lose its focus ring.

I modified the default behavior demo to have a div with some text inside around the trap, same as in your demo. Note that the demo page is styled such that anything that has focus has a big bold focus ring around it.

First is the "no tabindex" case, where you see that the div isn't focusable:

And even though the focus ring disappears from the anchors when I select text inside the trap, the focusIn event is never fired. I guess the anchor loses focus, but nothing gains focus? Seems strange since focus should go to the document, but 🤷‍♂️ not sure where it goes or doesn't go.

Next is the "tabindex = -1" case where the div is focusable. Notice the difference when and what things get focus:

The fact the ancestor div is focusable clearly interferes with what stuff gets clicked on, and you can imagine that once the trap is activated, clicking on the text inside the trap now registers click on the ancestor div, leading to the focusIn event because it's focusable, leading focus-trap to detect a focus escape even though you think you're just clicking on the text...

So all that to say... not a bug? What do you think? If you still think it's a focus-trap bug, how would you solve this?

from focus-trap.

Thanks for the detailed response!

So all that to say... not a bug? What do you think? If you still think it's a focus-trap bug, how would you solve this?

I guess my first question is whether focus-trap is intentionally also trapping text selection or not. Your first screencap also demonstrates surprising behavior, now that I think about it: why shouldn't you be able to select the "ancestor div without tabindex" text? Doing so wouldn't focus anything outside of the trap, after all. After some experimentation I see now that many interactions that have nothing to do with focus (like clicking on links or expanding/collapsing a <details> element) outside of the trap don't work.

If focus-trap is really more like user-interaction-trap then this behavior makes sense, but otherwise perhaps not.

Either way, it feels like within the trap you shouldn't be prevented from doing things when the trap is activated that you could have done otherwise, so I'm inclined to think the issue I originally described is a bug no matter what your thoughts are on the above (or perhaps it's a "known limitation" if there isn't a reasonable fix... I'd have to dig deeper into the implementation to have informed thoughts). But it's possible there's a bigger/related issue too, like "focus-trap interferes with user interactions which wouldn't otherwise have caused elements outside the trap to become focused".

from focus-trap.

Or perhaps the intention is "at all times there must be an element within the trap which has focus", in which case your first screencap demonstrates a different bug: the text inside there shouldn't be selectable in that scenario because doing so blurs all of the focusable elements.

from focus-trap.

Thanks for the detailed response!

You're welcome!

So all that to say... not a bug? What do you think? If you still think it's a focus-trap bug, how would you solve this?

I guess my first question is whether focus-trap is intentionally also trapping text selection or not.

Not intentionally. As I mentioned before, selecting text is a strange thing as it doesn't seem to affect focus, even though it kind of does. It seems to take focus away from something without then causing focus to go somewhere else, or at least without triggering normal focus-related events on that other thing.

Your first screencap also demonstrates surprising behavior, now that I think about it: why shouldn't you be able to select the "ancestor div without tabindex" text? Doing so wouldn't focus anything outside of the trap, after all.

That is a good point, at least so far, given my explanations/understanding of what happens (and doesn't happen) when merely selecting text.

After some experimentation I see now that many interactions that have nothing to do with focus (like clicking on links or expanding/collapsing a <details> element) outside of the trap don't work.

Anchors (i.e. links) do steal focus and are both tabbable and focusable by default. So that is a legitimate blocking by focus-trap to "keep the focus somewhere the trap".

Same thing for the expanding/collapsing <details> element: The expander is focusable and tabbable by default. You can try that out here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

Note that if you want a trap where you're still allowed to click on clickable stuff (note that text is not clickable) outside the trap, you can enable that and manage it on your own with the allowClickOutside(event) function option. But that only gets called on an actual click, which comes after focusIn. Still, a text node is not clickable (or it is, but since it's not focusable, I assume, you don't get a focusIn and a click for that), so you can't use this to "enable" selecting text outside the trap.

If focus-trap is really more like user-interaction-trap then this behavior makes sense, but otherwise perhaps not.

The aim of the library is to keep focus somewhere inside the trap. As long as focus-trap doesn't get the impression focus has escaped, it'll leave things alone. It doesn't even try to manage tab order -- other than if your trap has multiple containers, then it only watches to see if you're tabbing past one container and determines where focus should go depending on container order. That's it. Otherwise, the aim is to get out of the way -- but there are certain rules, as you're seeing.

It's not perfect. I expect/hope that in a few years, this library will actually start to fade into history as support for things like the new-ish inert attribute gains deeper browser version support. A native browser-supported solution would have a much better chance at addressing this text selection issue. Or maybe there will be a way focus-trap can add value to the use of inert. We'll see.

Either way, it feels like within the trap you shouldn't be prevented from doing things when the trap is activated that you could have done otherwise, so I'm inclined to think the issue I originally described is a bug no matter what your thoughts are on the above (or perhaps it's a "known limitation" if there isn't a reasonable fix... I'd have to dig deeper into the implementation to have informed thoughts). But it's possible there's a bigger/related issue too, like "focus-trap interferes with user interactions which wouldn't otherwise have caused elements outside the trap to become focused".

Fair enough. But I feel like we're glossing over one major point here: The fact that the way things work in HTML with parent elements where the parent is focusable and contains stuff you're expecting the user to also interact with. When that's the case, unless you specifically click on a child that can receive focus (and we've established that a text node child cannot), the parent will register a focus grab and a click.

As you can see, when the container of the trap is not focusable, I can select that text just fine while the trap inside of it is active:

Strange that my first animated demo above with "no tab index" makes it look like I can't select the text. Not sure why. I redid the video:

Or perhaps the intention is "at all times there must be an element within the trap which has focus", in which case your first screencap demonstrates a different bug: the text inside there shouldn't be selectable in that scenario because doing so blurs all of the focusable elements.

Not quite. The intention is to keep the focus somewhere inside the trap. That's it.

from focus-trap.

BTW, this is all I did to the demo in the index.html file:

Before:

        <div id="default" class="trap">
          <p>
            Here is a focus trap <a href="#">with</a> <a href="#">some</a> <a href="#">focusable</a> parts.
          </p>
          <p>
            <button id="deactivate-default" aria-describedby="default-heading">
              deactivate trap
            </button>
          </p>
        </div>

After:

      <div id="tabindex-test" style="border: 1px solid red; padding: 10px">
        ancestor div without tabindex
        <div id="default" class="trap">
          <p>
            Here is a focus trap <a href="#">with</a> <a href="#">some</a> <a href="#">focusable</a> parts.
          </p>
          <p>
            <button id="deactivate-default" aria-describedby="default-heading">
              deactivate trap
            </button>
          </p>
        </div>
      </div>

from focus-trap.

I will close this issue now. I think the problem is ultimately with the way things work in HTML with parent elements where the parent is focusable and contains stuff you're also expecting the user to interact with (i.e. a focusable parent that contains focusable children).

from focus-trap.

Apologies for losing track of this issue, but closing it seems fine. I may dig more deeply into the implementation at some point and if I come up with any ideas I'll open pull requests (and/or reopen this issue if discussion seems warranted).

Thanks again for being super responsive, and also for teaching me about the existence of inert—seems like that will be handy once it's more widely-supported (I've also had my eyes on <dialog> which I think might obviate the use cases we have for focus-trap, but migrating to it will be a pain).

from focus-trap.

Apologies for losing track of this issue, but closing it seems fine. I may dig more deeply into the implementation at some point and if I come up with any ideas I'll open pull requests (and/or reopen this issue if discussion seems warranted).

Sounds great!

Thanks again for being super responsive, and also for teaching me about the existence of inert—seems like that will be handy once it's more widely-supported (I've also had my eyes on which I think might obviate the use cases we have for focus-trap, but migrating to it will be a pain).

👍

from focus-trap.