Avoid walking entire parent tree unnecessarily in isElementOrAncestorHidden.

[alice]

This should help the audit run better on large pages with many hidden elements.

[alice]

@ricksbrown WDYT?

[ricksbrown]

@alice Hi there!

Funny stuff, my current branch on accessibility-developer-tools is called "hidden_na" - the idea was to ignore hidden elements for the purpose of the audit. I ended up abandoning the idea because of the results I got running it on some real pages. It turns out that I would have missed some genuine a11y issues if I had not audited hidden elements.

Edit: the reason it's worth auditing hidden elements is because in all likelihood they will become visible at some point (that's why they are part of the DOM in the first place).

So, personally I would like it to continue auditing hidden elements but maybe this could be configurable? I'd suggest default to the current behavior, audit hidden elements unless a rule specifically excludes itself because it knows it is not relevant when hidden.

In terms of addressing the performance issues, that's something I was meaning to ask you about. I would like the performance to reach a level where I could run the library in web applications during development and turn off before going to prod (and staging environments). This would need a performance tune because currently the overhead is quite noticeable.

If you would be interested I'd be happy to look into this if you like, perhaps we should raise a "performance" issue.

[alice]

@ricksbrown Good points there.
What issues would you have missed without auditing hidden elements? Agreed, the reason we didn't go down this road originally is exactly as you say: hidden elements will probably sometime be shown.

I modified this patch to remove the short-circuiting behaviour, but left the changes to isElementOrAncestorHidden because I think they will probably provide a performance benefit.

I'll also raise an issue for performance, and we can continue this discussion there.

[ricksbrown]

@alice The issues I would have missed were things we had done wrong in web systems where I work.
The nature of these issues generally was not affected by the element being hidden or visible and I definitely wanted to know about them.

Interestingly they were all hidden early in the page lifecycle (when the audit runs), waiting for some user interaction before they were visible. This is often the case with <details><summary>, menus, trees well lots of stuff really.

Two specific examples of what we would have missed:

• aria-readonly with role="radio"
• aria-describedby referring to an ID that did not (but should) exist

We also found we had aria-readonly with role="checkbox" - illegal in ARIA 1.0 but legal in ARIA 1.1 so there's another issue, when to adopt ARIA 1.1?
Allow it as a config option, let the user decide) or simply choose at some point and cut over.

[alice]

@ricksbrown Got it, makes sense.

How do you feel about this change without the short-circuit?

[ricksbrown]

@alice I like it...

buuuut I do think it needs unit tests because of the complexity of "hidden".
There are so many ways to "hide":

• aria-hidden
• display:none
• visibility: hidden
• HTML5 hidden="hidden"
• input type="hidden"
• zero dimension
• offscreen

Further complicated by the fact that an element itself may not be explicitly hidden but rather inherit it from a hidden ancestor.

I guess your thinking was that it is unit tested indirectly however I doubt this covers every relevant type of hidden. Sure there would be lots of aria-hidden tests but perhaps no html5 hidden.

[alice]

Yeah, I hear that.

Plus it turns out it doesn't make it any faster, anyway. Going to shelve this for now (although I do think I'll want to get this in eventually cause now that I've dug into it the current code looks wrong to begin with).