Bug 254981 – AX: Make help-text.html and list-detection2.html async-friendly so they can pass consistently in ITM

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Both of these tests dynamically assign an id to elements via JS without waiting for accessibility to update.

[Radar WebKit Bug Importer]

<rdar://problem/107600415>

[Tyler Wilcock]

Created attachment 465760 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 465760 [details]
> Patch

--- a/LayoutTests/accessibility/help-text.html
+++ b/LayoutTests/accessibility/help-text.html

+    for (var i = 0, c = examples.length; i < c; i++) {

examples.forEach would be more concise and elegant here.


+        var expectation = el.getAttribute("data-expected");
+        var description = platformValueForW3CDescription(ax);
+        if (expectation === expectation)

You probably meant expectation === description. Also I would use output += expect(...) instead of this if statement.

--- a/LayoutTests/accessibility/list-detection2.html
+++ b/LayoutTests/accessibility/list-detection2.html

+<script>
+window.jsTestIsAsync = true;
+</script>

We put this in the script tag of the body, not in the head.

Does the test use all that CSS in the header? If not, we should remove it.

+    var el, contentAttrRoleString, axElement, computedAriaRole, expectedRole, result, note;

Don't need to forward declare all these variables, declare them where used.

+            note = el.getAttribute("data-note")

Missing ;

+            result = el.tagName.toLowerCase() + (contentAttrRoleString ? ("[role=\""+contentAttrRoleString+"\"]") : "") + (note ? note : "");
+            result = `${el.tagName.toLowerCase()}${contentAttrRoleString ? `[role="${contentAttrRoleString}"]` : ""}${note ? `${note}` : ""}`;

???

+            if ((computedAriaRole == expectedRole))
+                output += `PASS: ${result}\n`;
+            else
+                output += `FAIL: Got ${result}, expected ${expectedRole}\n`;

Use output += expect().

+        // Once tests are complete, hide all the example markup.
+        examples = document.querySelectorAll(".ex, #examples");
+        for (var i = 0, c = examples.length; i < c; i++)
+            examples[i].style.display = "none";

Can we just hide examples without having to iterate?

[Tyler Wilcock]

Created attachment 465764 [details]
Patch

[Tyler Wilcock]

Created attachment 465765 [details]
Patch

[Tyler Wilcock]

> Also I would use output += expect(...) instead of this if statement.
I decided to leave these as-is because the output is nicer than if we used `expect`.

> Does the test use all that CSS in the header? If not, we should remove it.
Most of the CSS is necessary because CSS influences whether accessibility considers elements to be lists. I did remove some CSS that was unnecessary for the test, though.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> > Also I would use output += expect(...) instead of this if statement.
> I decided to leave these as-is because the output is nicer than if we used
> `expect`.

Test code as clean as possible saves time and effort, but not a big deal in this case. In my view, the output is much less important as long as text differences catch errors.
> 
> > Does the test use all that CSS in the header? If not, we should remove it.
> Most of the CSS is necessary because CSS influences whether accessibility
> considers elements to be lists. I did remove some CSS that was unnecessary
> for the test, though.

[EWS]

Committed 262601@main (2701cc7bd554): <https://commits.webkit.org/262601@main>

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