Bug 248104 – AX ITM: Fix for aria-hidden-changes-for-non-ignored-elements.html and html-with-aria-label.html in isolated tree mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Currently failing in ITM.

[Radar WebKit Bug Importer]

<rdar://problem/102528770>

[Andres Gonzalez]

Created attachment 463610 [details]
Patch

[Tyler Wilcock]

Comment on attachment 463610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463610&action=review

> LayoutTests/accessibility/mac/html-with-aria-label.html:20
> +        html.setAttribute("aria-label", "New Title");

I see this and some other places in this patch are using some implicit object-by-id JS syntax. I think this not recommended:

https://html.spec.whatwg.org/#named-access-on-the-window-object

> As a general rule, relying on this will lead to brittle code. Which IDs end up mapping to this API can vary over time, as new features are added to the web platform, for example. Instead of this, use document.getElementById() or document.querySelector().

> LayoutTests/accessibility/mac/html-with-aria-label.html:22
> +            output += expect("root.description", "'AXDescription: New Title'");

Do we need a `waitFor` here? Maybe moving the expect into the async block is enough...but I wonder if the test is flakey or will become flakey without an explicit waitFor.

[Andres Gonzalez]

Created attachment 463762 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 463610 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463610&action=review
> 
> > LayoutTests/accessibility/mac/html-with-aria-label.html:20
> > +        html.setAttribute("aria-label", "New Title");
> 
> I see this and some other places in this patch are using some implicit
> object-by-id JS syntax. I think this not recommended:
> 
> https://html.spec.whatwg.org/#named-access-on-the-window-object
> 
> > As a general rule, relying on this will lead to brittle code. Which IDs end up mapping to this API can vary over time, as new features are added to the web platform, for example. Instead of this, use document.getElementById() or document.querySelector().
> 
> > LayoutTests/accessibility/mac/html-with-aria-label.html:22
> > +            output += expect("root.description", "'AXDescription: New Title'");
> 
> Do we need a `waitFor` here? Maybe moving the expect into the async block is
> enough...but I wonder if the test is flakey or will become flakey without an
> explicit waitFor.

Didn't waitFor intentionally because doesn't seem to be necessary, no failures with --iterations=1000. In real scenarios, there may not be a wait at all, so want to reduce the use of waitFor to when it is strictly necessary. Having said that, will revisit if flakiness shows up in the bots.

[Tyler Wilcock]

> Didn't waitFor intentionally because doesn't seem to be necessary, no
> failures with --iterations=1000. In real scenarios, there may not be a wait
> at all, so want to reduce the use of waitFor to when it is strictly
> necessary. Having said that, will revisit if flakiness shows up in the bots.
OK, sounds good!

[EWS]

Committed 257113@main (557946b122c5): <https://commits.webkit.org/257113@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463762 [details].