Comments (14)
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 thenon-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 usingisNodeMatchingSelectorFocusable
). 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 thenon-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.
Related Issues (20)
- element.matches not working in ie 11. HOT 5
- Scroll containers HOT 3
- isTabbableRadio doesn't properly escape query selectors HOT 4
- Use @testing-library/dom and Jest for tests HOT 3
- Add code coverage badge to the README HOT 5
- Restore actual browser testing with Cypress HOT 8
- support inert attribute HOT 13
- Exclude form elements in `:disabled` fieldset HOT 2
- Optimizing displayCheck: 'full' HOT 9
- Bug: isDisabledFromFieldset doesn't check the top-most disabled <fieldset> HOT 3
- Safari: Object passed in getCheckedRadio is not iterable, causing a for loop error HOT 2
- v5.3.0 breaks literally all of our usage of `tabbable` 😅 when called on a node not attached to the document HOT 40
- [MAJOR] isHidden should exclude detached nodes by default
- Firefox throws an error when checking disabled on non button element HOT 4
- IE support broken in v5.3.2+ HOT 1
- tabbable breaks when processing an element with a "scope" attribute HOT 6
- jest error Your focus-trap must have at least one container with at least one tabbable node in it at all times HOT 2
- Can't make it work testing with tabbable mocked HOT 7
- jsdom issue: 'slot):not([inert]' is not a valid selector HOT 7
- Remove nwsapi v2.2.2 override once bug is fixed
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from tabbable.