Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable #1211

Merged
merged 9 commits into from
Nov 8, 2018

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Oct 29, 2018

This PR does the below:

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Marcy Sutton >>

@jeeyyy jeeyyy requested a review from a team as a code owner October 29, 2018 12:25
* @param {HTMLElement} el The HTML Element
* @return {Boolean} the element's visibility status
*/
dom.isCssVisible = function isCssVisible(el) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


const style = window.getComputedStyle(el, null);
if (!style) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just throw here instead, and have the check(s) catch it and resolve as incomplete? Seems more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may be a bit intrusive to throw for this reason. Happy to discuss.

return false;
}

const parent = el.assignedSlot ? el.assignedSlot : el.parentNode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably just use dom.getComposedParent

assert.isTrue(axe.commons.dom.isCssVisible(el));
});

it('should return true on detached elements', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should return false"

assert.isTrue(axe.commons.dom.isCssVisible(el));
});

it('should return true and not calculate position on parents', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the use of this test is.

assert.isFalse(axe.commons.dom.isCssVisible(el));
});

it('should correctly handle visible slotted elements', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should toggle this test if shadow DOM is not available, instead of passing it without assertions.

'</div>';

var el = document.getElementById('target');
assert.isFalse(axe.commons.dom.isCssVisible(el));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,153 @@
describe('dom.isCssVisible', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jeeyyy dismissed WilcoFiers’s stale review November 1, 2018 15:59

Updated. Review again.

@jeeyyy jeeyyy changed the title fix: introduce dom.isCssVisible function to be used in dom.isFocusable fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable Nov 1, 2018
@jeeyyy jeeyyy requested a review from a team November 1, 2018 16:39
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Nov 1, 2018

@WilcoFiers - ready for review.


const style = window.getComputedStyle(el, null);
if (!style) {
throw new Error('Style does not exist for the given element.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would this scenario occur?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this happens with unmounted elements.

Copy link
Contributor

@marcysutton marcysutton Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this case arise? Would these even be returned from querying the DOM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this happens with unmounted elements.

if (
(visibilityValue === 'hidden' && !descendentVisibilityValue) ||
(visibilityValue === 'hidden' &&
(descendentVisibilityValue && descendentVisibilityValue !== 'visible'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have tackled for the above.

@jeeyyy jeeyyy requested review from marcysutton, WilcoFiers and a team November 6, 2018 08:50
@jeeyyy jeeyyy changed the title fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable [WIP] fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable Nov 6, 2018
@jeeyyy jeeyyy changed the title [WIP] fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable fix: introduce dom.isHiddenWithCSS for use in dom.isFocusable Nov 6, 2018
@jeeyyy jeeyyy merged commit 2cff417 into develop Nov 8, 2018
@jeeyyy jeeyyy deleted the is-css-visible branch November 8, 2018 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants