Bug 254981

<rdar://problem/107600415>

Created attachment 465760 [details]
Patch

(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?

Created attachment 465764 [details]
Patch

Created attachment 465765 [details]
Patch

> 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.

(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.

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].