tabbable: v5.3.0 breaks literally all of our usage of `tabbable` 😅 when called on a node not attached to the document

This is truly OSS at its finest, you don’t see many respectful threads like these days 👏 👴

Just want to leave a thank you note for this, I’ve finally got some background on why all my tests were failing when migrated to tabbable@6 (via focus-trap@7) while everything seemed to be actually working on the live app (vue).

The test setup is vue + @vue/test-utils + jest + jsdom, so I suspect the issue is related to the fact that jsdom returns an empty array for getClientRects() event when attached. <= mocking this to return something else and combining it with delayInitialFocus: false prop that I learned from the focus-trap-react repo should make things back on track. (Will update this comment if confirmed)

For the moment, I’m using the legacy-full “escape hatch” for tests 😛 ✌️

EDIT: I don’t need to suspect, it is the reason, I just need to read the actual documentation chapters. 🤦

Thank you @stefcameron thats very nice. I could not be more pleased that the fix for this “beast” of a bug is #666 😂😂😈

I agree that the current behavior should be preserved as there are probably many that depends on it, but would change next major version to swap the default behavior to reflect the fact that detached nodes are not actually tabbable/focusable, with an option to ignore isHidden on detached nodes.

Love this discussion! 😆 Glad we can laugh a bit along the way, even with memes. Yes, what an adventure we are on in software land…

So I think we have solution, which is to assume that a detached node is visible. That should restore the old behavior, while letting us keep using the new optimization with getClientRects() when it’s attached.

I will update my PR tomorrow accordingly and will get a patch out.

With that perspective, I can see why you wouldn’t want to make that assumption in the library, though…

Option 1) un-break the library for any other existing consumers which may have a secret, bad dependency on this undocumented and weird behavior

Option 2) leave the new, correct behavior in 5.3.x, but break lots of consumers…

This bad set of alternatives is why I think reverting to 5.2.1 as 5.3.2, and then re-publishing 5.3.1 as 6.0.0, is the best option. Marking the “breaking” (i.e. fixing, lol) change to the API as a major version prevents any accidents.

So the real fix to this is to check if the node is detached, and then assume the node is visible – basically, @craigkovatch has been running in displayCheck: none all this time because the node was detached. Because when detached, getComputedStyle() returns undefined, but we were always checking === 'none' so we would conclude the node was visible.

I got it: I worked because it never worked in the first place.

@craigkovatch

I would suggest that since this has worked this way for many years now across five major versions, and only suddenly broke two weeks ago, it is in fact a part of the API, even if it was never explicitly documented so in the past.

That’s a very good point.

This is a very common misunderstanding of what componentDidMount means. It’s referring to “React has attached a component instance to a DOM container”, not that the DOM container has been attached to the document. (Actually, https://github.com/facebook/react/issues/9117#issuecomment-411590727 I had with the PM of React ~5 years ago about this very misconception, when I first learned the distinction 😃 but I think it’s a distraction from the change that occurred here; this is not a React problem.)

Thank you! This is indeed an easily overlooked detail about React I had never considered before, but makes a lot of sense.

My perspective is: you were absolutely right to fear breaking clients by making this change, and I hope that you’ll consider reverting it quickly and working out the details we’re discussing now for a future release.

Well, you know what they say about hindsight… 😉

Performance, especially in this isHidden() function in tabbable, has been a point of contention in the past (hence the displayMode option that’s relatively new for the lifespan of tabbable) and I was excited this might be a good solution, and seemed safe enough.

I like your suggestion of a document.body.contains(node) check and falling back to the old loop up the parent chain.

Trying this out locally…

I appreciate everyone’s input/discussion on this! The issue is patched in v5.3.2, and I’ve created an issue to fix the default behavior in the next major (though no plans to publish a major ATM). #666

restoring the old behaviour means that when a node is not attached to the DOM it is automatically considered tabbable as soon as it enters the isHidden() check.

I don’t see how this is the case

Sorry you are right, restoring the old behaviour only means to consider detached nodes visible, which is indeed a smaller problem.

Fix is available in tabbable 5.3.2 now.

@stefcameron yesterday I just trusted that you found a solution, without thinking too much about it. But now that I think about it, restoring the old behaviour means that when a node is not attached to the DOM it is automatically considered tabbable as soon as it enters the isHidden() check. This is not good.

Thanks for thinking about it. I don’t see how this is the case, however. If the node is attached, we’ll do the getClientRects() check. If the node is detached, we’ll assume it’s visible – once we get to the place in isHidden() where we would normally do the getClientRects() check.

That is the old behavior, because prior to getClientRects(), we were doing getComputedStyle(node).display === 'none' -> node IS hidden and when the node was detached, we would fail this check (because the display property would have undefined as a value instead of the computed CSS style) and determine the node was NOT hidden.

But no one was the wiser, probably because they didn’t have this special case where they had hidden nodes in a detached container they were expecting to be determined hidden, so not tabbable/focusable, and were getting what they thought may have been bad results from tabbable. Or they did get unexpected nodes back from tabbable and just learned to live with it or figured something else out, and they’ve been living with this for potentially years. So best to keep that expectation at this point.

For posterity – throwing everything behind a delay works fine at runtime, but still breaks all the downstream tests, because there’s now asynchronous behavior that a test I don’t own needs to be aware of. That’s specific to my context, of course, but wanted to fill out why sometimes it’s not so simple 😦

I updated the title in a meager effort toward helping more. I would love to add more, but I think the failure mode will be highly context-dependent and so it’s hard to come up with more “search keywords”.

In my case they just looked like a bunch of jasmine tests that timed out. I went binary searching through changes in modules between early January and now to eventually narrow it down to a change that updated ~12 dependencies, and then took a wild guess that it was tabbable.

@craigkovatch I didn’t mean to be dismissive, I apologize if I’m giving this impression. I was just relieved and optimistic because the fix is already done and will land soon.

I don’t have a broken codebase to deal with in the meanwhile though. I can understand that you are not going to feel relieved till the fix has been released.

If you want to help those that will look for the issue, maybe you can add a line to your original post pointing to some helpful comment (maybe you could write such comment for them).

@craigkovatch this is the only open issue on tabbable 😆 eventually they will arrive here and try some workaround waiting for the fix.

Anyway the bug doesn’t exist for those using react with function components and probably the same is true for those using frameworks other than react, so hopefully there are not so many broken projects out there 😅

I will try this out as a workaround for my current state of the world

If it can make your code work right now, why no?

But anyway, if I’ve understood correctly, the bug consisted in a simple missed check for whether an element is attached to the document or not, so it can be fixed easily.

Edit: oh yeah, there is still the problem to decide what displayCheck logic to use in case the element is detached.

@DaviDevMod

If you can, though, you should prefer to delay calling tabbable.

Yes, you’re right, that is probably the best option. I am loathe to move even more code into a setTimeout (or equivalent), but it’s kind of my fault for assuming that unattached nodes have any sensible definition of “tabbable” in the first place, I suppose 😃 I will try this out as a workaround for my current state of the world. I think still there’s some decisions to make for this library, though.

@DaviDevMod no angriness here 😃 I have caused this kind of problem just as often as I’ve been a victim of it. Any urgency in my words is in the interest of preventing someone else from hitting this who may be less able to figure out the cause. We’re all in this absurd software life together!

As an aside, any library of a single utility function that is nearing 1M+ downloads per week has a pretty strong argument to make about something missing in the browser. Perhaps we should work on a proposal to W3C to add :tabbable and :focusable as real selectors in the DOM/CSS spec(s). This is information the browser already has, at all times, and it seems like tabbable will always be doomed to playing catch-up, trying to reconstruct this knowledge from the “outside”.