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

[craigkovatch]

The change to use getClientRects().length completely breaks this library for us: #604

We use tabbable when initializing dialogs, to find candidate element(s) to auto-focus when the dialog appears. These dialogs are React components and the call to tabbable() occurs during the componentDidMount lifecycle method. At that moment, the DOM elements we want to traverse using tabbable are fully formed, but they are not yet attached to the document. So getClientRects().length always returns 0, and thus every single one of our calls to tabbable fails. (At this library level, we have no way to control when the dialog is actually attached to the document.)

Could this change be reverted, or can we scope it to only be used when document.body.contains(node)?

[craigkovatch]

As some additional context, this broke out from under us, as a result of declaring our dependency on tabbable as ^5.2.1 rather than pinned 5.2.1. When our dependents run yarn upgrade on our package, they also end up getting a broken version of tabbable with this bug.

I expect there are plenty of other projects which are also going to be broken in the same way -- next time they regenerate their lock files or upgrade a dependency which depends on tabbable, they will inherit this bug without changing anything or knowing why it suddenly broke.

[stefcameron]

Hey, thanks for reaching out! Just wondering, are you using focus-trap-react, or are you using tabbable on its own with the sole purpose of identifying focusable nodes before your dialog is rendered?

Also, how is the node not in the tree yet? The definition of componentDidMount is "componentDidMount() is invoked immediately after a component is mounted (inserted into the tree)." Refs should be available at this point as well, which should imply the node has been attached to the DOM tree.

(At this library level, we have no way to control when the dialog is actually attached to the document.)

But if you're not in control of this dialog (I assume, based on this, that it's a third-party library you're using, along with tabbable), so somehow it's still not attached to the DOM when componentDidUpdate fires (which, by React's definition of the API, it should be in the DOM), have you tried configuring tabbable with displayCheck: "none"?

That configuration would not rely on getClientRects(), but it also does not discriminate in any way on what is visible vs hidden.

Does this dialog library have a hook so you can tell when it's been attached to the DOM?

I expect there are plenty of other projects which are also going to be broken in the same way -- next time they regenerate their lock files or upgrade a dependency which depends on tabbable, they will inherit this bug without changing anything or knowing why it suddenly broke.

I understand it feels like a bug, but technically, tabbable has never sought to support cases where it's operating on an orphaned root node. It assumes the node is in the DOM. I think it just happened to be that it conveniently used to use getComputedStyle(node).display === 'none' which didn't rely on the node actually being in the DOM, and that API somehow was still able to compute that the resulting style is "none" even without knowing whether it might get mounted to a parent node in the DOM that has display: none, in which case it would become hidden (or it didn't, and everyone out there just happened to never insert it into such a hidden parent, and things "just worked").

But that doesn't mean there's nothing to be done about the change to smooth things over. Maybe we can still improve on it without just reverting or adding yet another displayCheck option nuance.

Right now, it feels like this dialog library is not React-based, or is but isn't playing by React rules, and tabbable is being asked to work around this, but maybe I'm missing something here. Please help me to understand if so!

[DaviDevMod]

Citing this answer on SO:

One drawback of using componentDidUpdate, or componentDidMount is that they are actually executed before the dom elements are done being drawn, but after they've been passed from React to the browser's DOM.

This problem does not exist in modern react, where componentDidMount and componentDidUpdate are superseded by useEffect.

Another answer to the same SO question suggests a fairly clean solution to the problem, that is to take advantage of a setState callback.

I am here sharing this information just because it pains me to know that my contribution broke something. Whether the problem should be addressed by tabbable is not my concern.

[craigkovatch]

@DaviDevMod That's not accurate. useEffect doesn't supersede those lifecycle methods, but it's the equivalent of those methods for function components (vs class components). The timing is the same, and the problem would be the same. The SO thread you linked is not relevant to the "deep in a support library" issue at hand -- but I do appreciate your response and empathy for the pain!

I would suggest let's not focus on the "React" part of this -- it's only incidental in that it's the thing causing the "node not attached to the DOM yet" timing, but that could occur outside React, too.

[craigkovatch]

@stefcameron We are using tabbable directly, since before it was under focus-trap, back from the davidtheclark days :) thank you for your quick engagement on this issue.

I understand it feels like a bug, but technically, tabbable has never sought to support cases where it's operating on an orphaned root node.

While I understand totally what you're saying here (as the author of an internal support library myself) -- 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. I would be quite shocked if I'm the only one who hits this issue -- probably I'm just the first to hit it and take the time to identify the cause.

I had to make an emergency patch to prevent dozens of dependent modules from suddenly failing at our company. So whether or not the tabbable API was intended to work this way previously -- 5.3.0 is a very-literally Breaking Change. At the very least I think you should document it as such, revert this change in a 5.3.2 release, and re-publish as 6.0.0, to prevent anyone else from stumbling into this.

Also, how is the node not in the tree yet?

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, here is a link to a discussion 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.)

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.

But if you're not in control of this dialog

I'm the author of the UI library which provides the Dialog component -- but I'm not in control of when the rendered dialog gets attached to the document.

[craigkovatch]

And although again I feel this should be first reverted and then improved upon, what did you think of my idea of running the "fast" version after a document.body.contains check, and falling back to the "slow" version otherwise?

[craigkovatch]

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".

[stefcameron]

@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, facebook/react#9117 (comment) 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...

[craigkovatch]

Thank you Stefan! Just to clarify, my comment about fearing breakage was not meant as a critique! :) Just camaraderie for the spidey sense about unexpected breakages, and how they usually come out of the most innocuous-seeming changes! 😆

[stefcameron]

Thank you Stefan! Just to clarify, my comment about fearing breakage was not meant as a critique! :) Just camaraderie for the spidey sense about unexpected breakages, and how they usually come out of the most innocuous-seeming changes! 😆

🤜 🤛

[DaviDevMod]

@craigkovatch

The timing is the same, and the problem would be the same.

useEffect is asynchronous, so it skips one event loop task, during which the DOM is painted.
There is a useLayoutEffect hook that is synchronous like componentDidMount.

Using the old loop is bad as it causes layout thrashing, but if it's the only option you have, it's better than nothing.
If you can, though, you should prefer to delay calling tabbable.

In any case, I feel your angriness, seeing your code breaking apart just by updating dependencies can easily be the worst nightmare of a developer.
Wish you to get rid of this issue ASAP.

[stefcameron]

Thanks for your input @DaviDevMod

@craigkovatch So I opened a draft #665 which falls back to the old traversal if the node isn't in the document at the time the check is being performed.

However, this is what I'm seeing: It doesn't work when the node is not in the document. getComputedStyle(node).display === undefined any time the node is not in the document. I checked manually in the browser by creating a node tree, adding it to the document, checking getComputedStyle() on a node, and I get "block" or "hidden" or whatever it is. Then I detach the container from the document, and run getComputedStyle() on that same node (now technically orphaned) and the API returned undefined.

If I roll back to 5.2.1, it actually works, whether the node is in or out of the document but I have no idea why, at least not right now. I don't see what could have happened that would now cause different behavior with getComputedStyle(). One major caveat to the tests back in 5.2.1 days was that they were running under jsDom, not Cypress as they are now. I suspect jsDom may have been giving false-positives.

But then it seemed to work for you.

But my manual test in the browser (latest Chrome on macOS) shows it doesn't work either.

So how in the world did this ever work?

Even if I rolled back to the original loop without the contains(node) check, it wouldn't work.

This comment on a somewhat related issue back in 2015 is in line with my manual test in the browser: "I don't see how the cloned node can have a computed style, as it isn't in the document." But I acknowledge this is not the exact same issue, though the cloned node isn't attached, so seems similar...

[stefcameron]

To be clear, when I say it worked under 5.2.1 code, I mean it worked in tests that weee jsdom-based which is questionable. We never had tests that checked that tabbable worked on detached containers, AFAICT.

[DaviDevMod]

If I roll back to 5.2.1, it actually works, whether the node is in or out of the document but I have no idea why

The displayCheck optimization shipped together with the shadow DOM feature. May it be possible that when an element is not yet attached to the actual DOM there is some shadow logic kicking in?

[stefcameron]

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

[craigkovatch]

@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!

[stefcameron]

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.

[craigkovatch]

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.

[craigkovatch]

@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]

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.

[craigkovatch]

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

It can be both :) I am concerned about others who may hit this.

(It also makes testing much more difficult, as the logic that throws an error if there's nothing tabbable is now asynchronous, which is considerably more difficult to catch in an async jasmine+React+enzyme world.)

[DaviDevMod]

@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 😅

[stefcameron]

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.

[craigkovatch]

Anyway the bug doesn't exist for those using react with function components

@DaviDevMod This is simply not correct, please read my reply above #664 (comment). (And even if it was, it's not an excuse.) React never guarantees a particular component is attached to the document. Using a hook-based lifecycle method in a functional component has no bearing on that. useLayoutEffect is synchronous on DOM mutation. React will happily mutate the DOM of an non-document-attached node and then synchronously call your hooks. The problem is the same, it does not make any difference if your component is implemented by a class or a function.

[DaviDevMod]

@craigkovatch It's true that you can reproduce the bug in function components using useLayoutEffect, but you also have the choice to avoid the bug by using useEffect, which anyone would use in any case (useLayoutEffect has just a few specific use cases).

gaearon suggested a workaround in the thread you linked because at the time there were no hooks around. https://www.google.com/search?q=react+16.8+release+date

In any case the fix is coming! 🎉

[craigkovatch]

@DaviDevMod sorry, none of that is true, and I don't want to argue about it anymore. It's not that simple. (e.g. his workaround is only relevant if you own the entire app your code is running within. I don't.)

this is the only open issue on tabbable

It took me nearly two hours to determine the source of the build breakage in the ~25 downstream modules that suddenly broke as a result of this change. Again, I'm not angry about it, but in large and complex software projects, it's often non-trivial to determine what broke, let alone how it broke. I feel you're being a bit dismissive at this point, and I ask that you consider that there's lots of context on literally a million different dependents of this library that none of the three of us understand. It's important to try to Do The Right Thing for them, even if the diagnosis here is now clear. If we can prevent other people from spending a day on this, that's a better path than making it easy to fix only after they are broken and eventually find their way here.

[DaviDevMod]

@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]

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]

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 :(

[stefcameron]

OK, well, that was not as easy as it looked given the added complexity of shadow DOM support, but I think I've got this fixed. Hopefully the patch will be good. #665

[DaviDevMod]

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.

@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.

The possibilities I can see here are:

• Keep the old behaviour, meaning that if a node is detached it's always tabbable (you may want to document it somewhere)
• Declare that tabbabble can't be given as input detached nodes (this would break the code of people that were calling tabbable with detached nodes)
• Add support for detached nodes.

The last two options are not necessarily a breaking change, since the old behaviour for detached nodes wasn't even intended, it just happened to be somehow exploitable.

Regarding the last option, I think that the only way would be introducing async code in tabbable, because it's not possible to asses whether a detached node is focusable and/or tabbable. So the only way is to wait until the node is attached.

I wrote some code to see what the async logic would look liike. Here is the added logic for isFocusable, the same addition would need to be done in isTabbable, focusable and tabbable.

const isFocusable = function (node, options, tries) {
  if (!document.body.includes(node)) {
    if (tries > 9) throw new Error(node + 'is not attached to the DOM.');
    return async () => isFocusable(node, options, tries + 1);
  }

...rest of the function...
}

The value of 9 is totally arbitrary and may as well be 1, which can be implemented as a boolean.

I abstain from giving opinions on what should be done, I just wanted to share my reasoning on the issue.

[stefcameron]

@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.

[stefcameron]

Fix is available in tabbable 5.3.2 now.

[idoros]

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.

[DaviDevMod]

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.

[stefcameron]

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

[stefcameron]

@all-contributors add @craigkovatch for bug

[allcontributors]

@stefcameron

I've put up a pull request to add @craigkovatch! 🎉

[craigkovatch]

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

[renatodeleao]

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. 🤦