getComputedStyled() causes a forced reflow about tabbable HOT 14 CLOSED

For those, I wonder if we could get away with just looking at the style attribute instead of using the function.

from tabbable.

Hi, looking at the code I think the issue might be calling getComputedStyle on all ancestors here.

A workaround could be to check if the element's bounding client rect has a zero size, e.g:

const hasZeroSize = node => {
  const {width, height} = node.getBoundingClientRect()
  return width === 0 && height === 0
}
const isVisuallyHidden = node => getComputedStyle(node).visibility === 'hidden'
const isHidden = node => (
  isFirstDirectSummaryInClosedDetails(node)
  || hasZeroSize(node)
  || isVisuallyHidden(node)
)

This would at least prevent style / layout recalc on the whole document...

from tabbable.

This change is a breaking change and in my opinion there are cases where focusable elements have zero size, like skip to main content link for accessibility that is not visible until focused (first tab on sky news or double-tab at the beginning of bbc.com for example).

Edit: my examples have size even when not visible, however I still think this is a breaking change because browsers do give focus to elements with zero size and this change diverge from this behavior.

getComputedStyle should be lazy and shouldn't cause reflow without accessing any layout properties, so the performance issue is not directly related to getComputedStyle, but to the traversal and check up the DOM.

from tabbable.

This change is a breaking change

Indeed, although the tests pass.

In my opinion there are cases where focusable elements have zero size, like skip to main content link for accessibility that is not visible until focused

In this particular case, no. Having a zero-size element with focusable content is considered a strong accessibility anti-pattern.
That's why most if not all websites implements this using a 1px by 1px element which is then clipped and/or moved out of the document w/ absolute positioning, i.e:

.visually-hidden {
  overflow: hidden;
  position: absolute;
  height: 1px;
  width: 1px;
  padding: 0;
  border: 0;
  clip: rect(0, 0, 0, 0);
}

getComputedStyle should be lazy and shouldn't cause reflow without accessing any layout properties

How I which it where true ! Unfortunately that is definitely not the case. Reading any property of the object returned by getComputedStyle will cause a style recalculation and most probably a reflow if the styles where changed since the last call to getComputedStyle.

so the performance issue is not directly related to getComputedStyle, but to the traversal and check up the DOM.

Well walking the DOM doesn't cause layout thrashing, so it is directly related to getComputedStyle. Since you have to use it to read the node visibility anyway, the best you can do is limit the scope of potential recomputations by avoiding calling it for the whole ancestor hierarchy.

from tabbable.

Fair enough, I still think this is a breaking change that should maybe be implemented as an optional flag (not default), and flipped on the next major version.

from tabbable.

Good discussion. I like that we're thinking a bit outside the box with @ju1ius's solution, but it does feel like we should probably be more caution and put this behind a flag on the options object as opt-in behavior, and keep the current behavior as default and see how the rest of the user base likes it.

from tabbable.

I wonder if we could solve this for the few that find it a problem by simply adding a flag that would bypass the only 2 getComputedStyle() calls in isHidden and assume all nodes are visible. In the few cases I've heard about where using this library causes performance issues, perhaps the consumers of the libraries know their code better than this library does, and they can certify that assumption to be true?

from tabbable.

The PR that was opened seems fine to me, it just missed the flag. I don't mind completing it you want.

from tabbable.

Thanks, I appreciate that, but the PR was using getBoundingClientRect() which also causes a reflow of the layout, so is it really going to solve anything, or are we putting this in just to split hairs?

I get that the PR now uses this method first and avoids using getComputedStyle() thanks to short-circuit eval, but the existing code also basically short-circuits after the first call to getComputedStyle() if it finds the node has visibility: hidden.

🤔

from tabbable.

The alternative was to prevent going up the DOM and calling getComputedStyle() on each parent element, assuming the a zero bounding rect means that the element (or one of its ancestors is not visible). That is much less intensive.

How about a config with 3 alternatives: visibilityCheck: full/non-zero-bounds/none (not sure about the names)?

from tabbable.

The alternative was to prevent going up the DOM and calling getComputedStyle() on each parent element, assuming the a zero bounding rect means that the element (or one of its ancestors is not visible). That is much less intensive.

Ah, yes, thanks for the refresher. Right, we've replaced N calls to getComputedStyle() up the hierarchy with a single call to getBoundingClientRect() to check if it has a zero area.

How about a config with 3 alternatives: visibilityCheck: full/non-zero-bounds/none (not sure about the names)?

I think that could work. I can't come-up with better names so far, except maybe "non-zero-area". Obviously, the default would be "full" to maintain backward compatibility.

What about the guidance in the README for the new option? WDYT about these definitions?

• "full": This check may cause significant layout reflow and performance degradation when looking for focusable/tabbable nodes in a container, especially one deep in the DOM hierarchy, however, it's the most reliable option as it makes no assumptions. Default when option is not specified.
• "non-zero-area": This check assumes any nodes that have a zero area (width or height is zero) are non-focusable/tabbable. It will still cause layout reflow, but depending on the depth of your DOM hierarchy, it may be significantly faster than the "full" option. Use this only if you don't use visibly-hidden nodes for anything related to user interaction (some accessibility tricks use this technique).
• "none": This check assumes you know your DOM and CSS better than tabbable does and does not perform any visibility checks that would cause layout reflow. Use this if you need to squeeze the most performance out of your app and you can avoid using hidden nodes in any focusable/tabbable checks.

Finally, we'll need to surface the same option up in focus-trap so that traps can leverage this. focus-trap-react will get it for free once focus-trap provides it.

from tabbable.

My mistake, it's a display check, not visibility.

This change needs to add options to all of tabbable exported apis (currently isTabbable, isFocusable don't get any options).

The biggest issue with implementing this is that there is currently no pretty way to pass options into the filter functions, and there is reuse of filter functions internally (isNodeMatchingSelectorTabbable is using isNodeMatchingSelectorFocusable). So a few ways we can go with:

• set options in the module closure while running a check (easy/fast/dirty?)
• add a first parameter for options to all filter functions and bind/call them depending on the usage (ugly?)
• add a second parameter for options to all filter functions, anonymous function around filter to pass the options (less ugly)

What about the guidance in the README for the new option?

• I would soften the severity of the warning (it very much depends how you use the lib and in most cases the implications are probably minimal).
• I think the accessibility difference can be emphasised between full/non-zero-area (full will return more elements that are tabbable, but are less valid for accessibility).
• I think none is probably always a bad choice, if you know your DOM so well then you would probably have the list of tabbable elements hardcoded anyway. The performance different is so negligible from the non-zero-size that this might have a good chance of causing accessibility bugs and we should probably not encourage its usage by writing things like "squeeze the most performance out of your app".

Finally, we'll need to surface the same option up in focus-trap

Let's finish this minor change first

from tabbable.

This change needs to add options to all of tabbable exported apis (currently isTabbable, isFocusable don't get any options).

The biggest issue with implementing this is that there is currently no pretty way to pass options into the filter functions, and there is reuse of filter functions internally (isNodeMatchingSelectorTabbable is using isNodeMatchingSelectorFocusable). So a few ways we can go with:

• set options in the module closure while running a check (easy/fast/dirty?)
• add a first parameter for options to all filter functions and bind/call them depending on the usage (ugly?)
• add a second parameter for options to all filter functions, anonymous function around filter to pass the options (less ugly)

Right, tabbable() and focusable() already take options, but isTabbable() and isFocusable() do not. I think we can add a second parameter to isTabbable() and isFocusable() to accept options.

As for the internal isNodeMatchingSelectorTabbable() and isNodeMatchingSelectorFocusable() calls, I think the safest approach would be to insert a first parameter as "options" and then bind the options to them in tabbable() and focusable() when they're used as filter functions.

Wrapping with anonymous functions in the filter cases is a solution too, but IMO no more or less uglier than using bind(). Binding doesn't seem all that ugly to me, and this is internal anyway. Externally, our 4 APIs will be consistent, with first parameter an element, and second parameter, optional options.

Using module closure variables while running checks could lead to concurrency issues if any of the tabbable public APIs were run in an async promise handler, for example.

What about the guidance in the README for the new option?

• I would soften the severity of the warning (it very much depends how you use the lib and in most cases the implications are probably minimal).

OK

• I think the accessibility difference can be emphasised between full/non-zero-area (full will return more elements that are tabbable, but are less valid for accessibility).

I like that.

• I think none is probably always a bad choice, if you know your DOM so well then you would probably have the list of tabbable elements hardcoded anyway. The performance different is so negligible from the non-zero-size that this might have a good chance of causing accessibility bugs and we should probably not encourage its usage by writing things like "squeeze the most performance out of your app".

I see your point, but then we don't have a way to feed the list of tabbable elements to tabbable, or to focus-trap, so even if you know the list of tabbable elements in the DOM, you're still relying on focus-trap/tabbable finding the same set. If we're providing this option, then I think we're acknowledging that even the middle of the road "non-zero-area" option will still result in a performance hit which you might want/need to avoid in some extreme cases.

But we should emphasize that "none" is likely a bad choice unless you really know what you're doing, or something like that.

Finally, we'll need to surface the same option up in focus-trap

Let's finish this minor change first

For sure! One thing at a time. But it will need to trickle up, so I want to keep it in mind so what we do here works well when we get to that. And it's a note-to-self that after this tabbable update, it won't just be a version bump in focus-trap... 😄

from tabbable.

Fixed with #286, closing

from tabbable.