Bug 134187 – AX: improve list heuristics (presentational use versus actual lists)

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

[James Craig]

AX: improve list heuristics (presentational use versus actual lists)

Attaching a layout test that should better clarify what WebKit should exposed as a "list" to the accessibility APIs. The expected rolename "generic" of the non-list examples may need to change, but the rest is solid I believe.

[Radar WebKit Bug Importer]

<rdar://problem/17415885>

[James Craig]

Created attachment 233587 [details]
layout test

[chris fleizach]

Created attachment 253470 [details]
patch

[Darin Adler]

Comment on attachment 253470 [details]
patch

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

Test coverage is critical, because the accessibility code is complicated and much different from the normal DOM and rendering of these elements.

> Source/WebCore/accessibility/AccessibilityList.cpp:104
> +    Element* listItemElement = is<Element>(listItem->node()) ? downcast<Element>(listItem->node()) : nullptr;

Not sure what the point of the is<Element> check here, because ...

> Source/WebCore/accessibility/AccessibilityList.cpp:105
> +    if (!listItemElement->beforePseudoElement())

here we would dereference the null pointer. I think this needs an additional null check.

[chris fleizach]

Comment on attachment 253470 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityList.cpp:105
>> +    if (!listItemElement->beforePseudoElement())
> 
> here we would dereference the null pointer. I think this needs an additional null check.

i didn't want to downcast to Element unless it was an Element. I guess that's taken care of by the downcast

good catch. thanks

[chris fleizach]

http://trac.webkit.org/changeset/184676

[James Craig]

Comment on attachment 253470 [details]
patch

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

> LayoutTests/accessibility/list-detection2-expected.txt:25
> +

Strange that the "TEST COMPLETE" line came out before the results. Was that an error in my layout test?

[chris fleizach]

(In reply to comment #7)
> Comment on attachment 253470 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253470&action=review
> 
> > LayoutTests/accessibility/list-detection2-expected.txt:25
> > +
> 
> Strange that the "TEST COMPLETE" line came out before the results. Was that
> an error in my layout test?

The before:: content image stuff didn't load early enough for the script to run so i had to run the test after the body finished loading

[James Craig]

(In reply to comment #8)

> The before:: content image stuff didn't load early enough for the script to
> run so i had to run the test after the body finished loading

Makes sense. Thanks for the additional info.

[Alexey Proskuryakov]

The new test fails on all non-Mac platforms:

 TEST COMPLETE
-PASS: ul[role="list"] w/ explicit role and displayed inline -> list. 
-PASS: ul[role="list"] w/ explicit role but no markers -> list. 
-PASS: ul w/ default list markers -> list. 
-PASS: ol w/ default list counters -> list. 
-PASS: ul w/ list-style-image -> list. 
-PASS: ul w/ image content on ::before -> list. 
-PASS: ul w/ image content on inline ::before -> list. 
-PASS: ul w/ bullet content on ::before -> list. 
-PASS: ul w/ bullet content on inline ::before -> list. 
-PASS: ol w/ counter content on ::before -> list. 
-PASS: ol w/ counter content on inline ::before -> list. 
-PASS: ul w/ background image (NOT A LIST) -> group. 
-PASS: ul w/ background on ::before (NOT A LIST) -> group. 
-PASS: ul w/o explicit role and displayed inline, which defaults to no markers (NOT A LIST) -> group. 
-PASS: ol w/o explicit role and displayed inline, which defaults to no markers (NOT A LIST) -> group. 
-PASS: ul w/o explicit role and no markers (NOT A LIST) -> group. 
+FAIL: ul[role="list"] w/ explicit role and displayed inline -> . Expected: list.
+FAIL: ul[role="list"] w/ explicit role but no markers -> . Expected: list.
+FAIL: ul w/ default list markers -> . Expected: list.
+FAIL: ol w/ default list counters -> . Expected: list.
+FAIL: ul w/ list-style-image -> . Expected: list.
+FAIL: ul w/ image content on ::before -> . Expected: list.
+FAIL: ul w/ image content on inline ::before -> . Expected: list.
+FAIL: ul w/ bullet content on ::before -> . Expected: list.
+FAIL: ul w/ bullet content on inline ::before -> . Expected: list.
+FAIL: ol w/ counter content on ::before -> . Expected: list.
+FAIL: ol w/ counter content on inline ::before -> . Expected: list.
+FAIL: ul w/ background image (NOT A LIST) -> . Expected: group.
+FAIL: ul w/ background on ::before (NOT A LIST) -> . Expected: group.
+FAIL: ul w/o explicit role and displayed inline, which defaults to no markers (NOT A LIST) -> . Expected: group.
+FAIL: ol w/o explicit role and displayed inline, which defaults to no markers (NOT A LIST) -> . Expected: group.
+FAIL: ul w/o explicit role and no markers (NOT A LIST) -> . Expected: group.

[chris fleizach]

(In reply to comment #10)
> The new test fails on all non-Mac platforms:
> 
>  TEST COMPLETE
> -PASS: ul[role="list"] w/ explicit role and displayed inline -> list. 
> -PASS: ul[role="list"] w/ explicit role but no markers -> list. 
> -PASS: ul w/ default list markers -> list. 
> -PASS: ol w/ default list counters -> list. 
> -PASS: ul w/ list-style-image -> list. 
> -PASS: ul w/ image content on ::before -> list. 
> -PASS: ul w/ image content on inline ::before -> list. 
> -PASS: ul w/ bullet content on ::before -> list. 
> -PASS: ul w/ bullet content on inline ::before -> list. 
> -PASS: ol w/ counter content on ::before -> list. 
> -PASS: ol w/ counter content on inline ::before -> list. 
> -PASS: ul w/ background image (NOT A LIST) -> group. 
> -PASS: ul w/ background on ::before (NOT A LIST) -> group. 
> -PASS: ul w/o explicit role and displayed inline, which defaults to no
> markers (NOT A LIST) -> group. 
> -PASS: ol w/o explicit role and displayed inline, which defaults to no
> markers (NOT A LIST) -> group. 
> -PASS: ul w/o explicit role and no markers (NOT A LIST) -> group. 
> +FAIL: ul[role="list"] w/ explicit role and displayed inline -> . Expected:
> list.
> +FAIL: ul[role="list"] w/ explicit role but no markers -> . Expected: list.
> +FAIL: ul w/ default list markers -> . Expected: list.
> +FAIL: ol w/ default list counters -> . Expected: list.
> +FAIL: ul w/ list-style-image -> . Expected: list.
> +FAIL: ul w/ image content on ::before -> . Expected: list.
> +FAIL: ul w/ image content on inline ::before -> . Expected: list.
> +FAIL: ul w/ bullet content on ::before -> . Expected: list.
> +FAIL: ul w/ bullet content on inline ::before -> . Expected: list.
> +FAIL: ol w/ counter content on ::before -> . Expected: list.
> +FAIL: ol w/ counter content on inline ::before -> . Expected: list.
> +FAIL: ul w/ background image (NOT A LIST) -> . Expected: group.
> +FAIL: ul w/ background on ::before (NOT A LIST) -> . Expected: group.
> +FAIL: ul w/o explicit role and displayed inline, which defaults to no
> markers (NOT A LIST) -> . Expected: group.
> +FAIL: ol w/o explicit role and displayed inline, which defaults to no
> markers (NOT A LIST) -> . Expected: group.
> +FAIL: ul w/o explicit role and no markers (NOT A LIST) -> . Expected: group.

Looking into it

[chris fleizach]

http://trac.webkit.org/changeset/184721

[James Craig]

*** Bug 170179 has been marked as a duplicate of this bug. ***