Bug 134187 - AX: improve list heuristics (presentational use versus actual lists)
Summary: AX: improve list heuristics (presentational use versus actual lists)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
: 170179 (view as bug list)
Depends on:
Blocks: 134188 130289
  Show dependency treegraph
 
Reported: 2014-06-23 01:14 PDT by James Craig
Modified: 2018-05-16 02:51 PDT (History)
13 users (show)

See Also:


Attachments
layout test (7.60 KB, text/html)
2014-06-23 01:15 PDT, James Craig
no flags Details
patch (16.38 KB, patch)
2015-05-20 16:07 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-06-23 01:14:45 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-06-23 01:15:00 PDT
<rdar://problem/17415885>
Comment 2 James Craig 2014-06-23 01:15:44 PDT
Created attachment 233587 [details]
layout test
Comment 3 chris fleizach 2015-05-20 16:07:02 PDT
Created attachment 253470 [details]
patch
Comment 4 Darin Adler 2015-05-20 16:13:01 PDT
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.
Comment 5 chris fleizach 2015-05-20 16:32:47 PDT
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
Comment 6 chris fleizach 2015-05-20 16:43:09 PDT
http://trac.webkit.org/changeset/184676
Comment 7 James Craig 2015-05-21 00:08:16 PDT
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?
Comment 8 chris fleizach 2015-05-21 00:14:53 PDT
(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
Comment 9 James Craig 2015-05-21 00:28:40 PDT
(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.
Comment 10 Alexey Proskuryakov 2015-05-21 09:43:57 PDT
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.
Comment 11 chris fleizach 2015-05-21 09:51:31 PDT
(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
Comment 12 chris fleizach 2015-05-21 10:46:12 PDT
http://trac.webkit.org/changeset/184721
Comment 13 James Craig 2018-05-16 02:51:09 PDT
*** Bug 170179 has been marked as a duplicate of this bug. ***