fix(aria-hidden-focusable): disabled aria-hidden fieldset should not have focusable children #3056
Conversation
| @@ -41,6 +41,50 @@ describe('focusable-disabled', function() { | |||
| assert.isTrue(actual); | |||
| }); | |||
|
|
|||
| it('returns true when content made unfocusable through disabled fieldset', function() { | |||
| var params = checkSetup( | |||
| '<fieldset disabled aria-hidden="true"><input id="target" /></fieldset>' | |||
There was a problem hiding this comment.
These tests are technically running against the wrong node. The evaluate function should be run against the element that has the aria-hidden on it. Each of these tests (except for the shadow root one) runs against an input element. This can potentially cause problems later on if something about how we get tabbableElements stops returning the node itself.
lib/commons/dom/focus-disabled.js
Outdated
| @@ -14,6 +14,23 @@ function focusDisabled(el) { | |||
| return true; | |||
| } | |||
|
|
|||
| let parentNode = vNode.parent; | |||
| while (parentNode) { | |||
There was a problem hiding this comment.
Looking at this more closely this will cause a pretty big performance problem. Since each child element of the aria-hidden element will use this function, each of them will walk up the entire parent tree of the page. If the aria-hidden is placed near the top of the tree, that is a lot of checking of nodes already checked.
We should cache on the VirtualNode information about this lookup so each element only needs to go up a few nodes until it finds an element that has already gone through this lookup chain.
We'll want to do something similar to is-hidden and cache the results of the recursive lookup onto each node we look at so we only ever have to look at a parent tree once.
There was a problem hiding this comment.
The cache on focusDisabled isn't quite working. The cache won't break early from looking at the entire parent tree looking for a fieldset. What you'll need to do is look at at _isDisabled for every parent element and if it isn't undefined return the value. You'll also then want to cache that value on the node itself so a child of the node will only have to go up 1 parent to find the result.
And since this is specific to a fieldset parent being disabled, you'll probably want to name the variable _fieldsetDisabled or _fieldsetParentDisabled or something like.
lib/commons/dom/focus-disabled.js
Outdated
| let parentNode = vNode.parent; | ||
| const ancestors = []; | ||
| let fieldsetDisabled = false; | ||
| while (parentNode && !fieldsetDisabled) { |
There was a problem hiding this comment.
If we know we're not in the same shadow tree anymore we can stop looking at the parent tree completely as well. This also removes the need to check for this in the if statements as well.
| while (parentNode && !fieldsetDisabled) { | |
| while (parentNode && parentNode.shadowId === vNode.shadowId && !fieldsetDisabled) { |
There was a problem hiding this comment.
Also, we'll want to break once we know whether or not there is a parent fieldset disabled. Right now this only breaks if the value is true, but we want to short circuit the entire tree as soon as any node has the cached value regardless of what the value is.
Closes issue: #2961