fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable #1211
Conversation
lib/commons/dom/is-css-visible.js
Outdated
| * @param {HTMLElement} el The HTML Element | ||
| * @return {Boolean} the element's visibility status | ||
| */ | ||
| dom.isCssVisible = function isCssVisible(el) { |
There was a problem hiding this comment.
I'm not sure about the name of this thing. It's not especially clear. How about we flip this around and call it dom.isHiddenWithCSS?
lib/commons/dom/is-css-visible.js
Outdated
|
|
||
| const style = window.getComputedStyle(el, null); | ||
| if (!style) { | ||
| return false; |
There was a problem hiding this comment.
Should we just throw here instead, and have the check(s) catch it and resolve as incomplete? Seems more appropriate.
There was a problem hiding this comment.
I think this may be a bit intrusive to throw for this reason. Happy to discuss.
lib/commons/dom/is-css-visible.js
Outdated
| return false; | ||
| } | ||
|
|
||
| const parent = el.assignedSlot ? el.assignedSlot : el.parentNode; |
There was a problem hiding this comment.
Should probably just use dom.getComposedParent
test/commons/dom/is-css-visible.js
Outdated
| assert.isTrue(axe.commons.dom.isCssVisible(el)); | ||
| }); | ||
|
|
||
| it('should return true on detached elements', function() { |
test/commons/dom/is-css-visible.js
Outdated
| assert.isTrue(axe.commons.dom.isCssVisible(el)); | ||
| }); | ||
|
|
||
| it('should return true and not calculate position on parents', function() { |
There was a problem hiding this comment.
I don't know what the use of this test is.
test/commons/dom/is-css-visible.js
Outdated
| assert.isFalse(axe.commons.dom.isCssVisible(el)); | ||
| }); | ||
|
|
||
| it('should correctly handle visible slotted elements', function() { |
There was a problem hiding this comment.
We should toggle this test if shadow DOM is not available, instead of passing it without assertions.
test/commons/dom/is-css-visible.js
Outdated
| '</div>'; | ||
|
|
||
| var el = document.getElementById('target'); | ||
| assert.isFalse(axe.commons.dom.isCssVisible(el)); |
There was a problem hiding this comment.
This should be true. Unlike display:none, which is a "once hidden, always hidden" kind of thing, you can make things visible inside of hidden regions.
test/commons/dom/is-css-visible.js
Outdated
| @@ -0,0 +1,153 @@ | |||
| describe('dom.isCssVisible', function() { | |||
There was a problem hiding this comment.
This should have display:none tests that run even without shadow DOM. Also, the display:none test should check that if an ancestor is set to display:none, that changing display of its dependents doesn't show make them visible again (unlike with visibility).
jeeyyy
changed the title
|
@WilcoFiers - ready for review. |
|
|
||
| const style = window.getComputedStyle(el, null); | ||
| if (!style) { | ||
| throw new Error('Style does not exist for the given element.'); |
There was a problem hiding this comment.
Where would this scenario occur?
There was a problem hiding this comment.
I think this happens with unmounted elements.
There was a problem hiding this comment.
I don't even know how that would come up in the context of an audit, but ok :)
edit: maybe in a unit test where the context wasn't mounted? A lot of our other rules would have problems though.
| * @param {Boolean} descendentVisibilityValue (Optional) immediate descendant visibility value used for recursive computation | ||
| * @return {Boolean} the element's hidden status | ||
| */ | ||
| dom.isHiddenWithCSS = function isHiddenWithCSS(el, descendentVisibilityValue) { |
There was a problem hiding this comment.
I like the new name for this method. 👍
Are you pulling any of the code out of is-visible, or is that mostly duplicate at this point?
There was a problem hiding this comment.
This has some replication from is-visible and is-hidden. We may need to do some refactor afterwards.
| assert.isFalse(actual); | ||
| }); | ||
|
|
||
| it('should return false on detached elements', function() { |
There was a problem hiding this comment.
When would this case arise? Would these even be returned from querying the DOM?
There was a problem hiding this comment.
You are right, probably won't be returned when querying the DOM. No harm in this test case, as that is exactly what we are testing.
|
|
||
| const style = window.getComputedStyle(el, null); | ||
| if (!style) { | ||
| throw new Error('Style does not exist for the given element.'); |
There was a problem hiding this comment.
I think this happens with unmounted elements.
| if ( | ||
| (visibilityValue === 'hidden' && !descendentVisibilityValue) || | ||
| (visibilityValue === 'hidden' && | ||
| (descendentVisibilityValue && descendentVisibilityValue !== 'visible')) |
There was a problem hiding this comment.
should be === 'hidden' There are more values on CSS visibility than just hidden and visible.
|
|
||
| const parent = dom.getComposedParent(el); | ||
| if (parent) { | ||
| return dom.isHiddenWithCSS(parent, visibilityValue); |
There was a problem hiding this comment.
Nice try on the recursion, but it doesn't work this way. This function shows #foo as hidden, which it isn't:
<div style="visibility:hidden">
<div style="visibility:hidden">
<div style="visibility:visible" id="foo">
Visible
</div>
</div>
</div>There was a problem hiding this comment.
Have tackled for the above.
…e dom.isHiddenWithCSS
jeeyyy
changed the title
jeeyyy
changed the title
This PR does the below:
isCssVisibleto satisfy requirements from issue Update dom.isFocusable function (PR#1211) #1208dom.isVisibleusage in focus computation withdom.isCssVisiblearia-hidden-focus- feat: rule aria-hidden-focus #1166, which exists as a WIP PR.Closes issue:
Reviewer checks
Required fields, to be filled out by PR reviewer(s)