Bug 236777

Summary: AX: List item marker not exposed when not a direct child of a list item
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: AccessibilityAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, aperez, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, kkinnunen, samuel_white, webkit-bug-importer
Priority: P2 Keywords: Gtk, InRadar, LayoutTestFailure
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=245071
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch for landing none

Description Carlos Garcia Campos 2022-02-17 05:53:46 PST
It can happen that the marker is not a direct child of a list item, for example in this case:

<ol>
  <li>
    <a href="#">
      <span style="display:block;">first block span</span>
      <span>inline span</span>
      <span style="display:block;">second block span</span>
    </a>
  </li>
</ol>

the render tree is:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x576
      RenderBlock {OL} at (0,0) size 784x54
        RenderListItem {LI} at (40,0) size 744x54
          RenderBlock (anonymous) at (0,0) size 744x0
            RenderInline {A} at (0,0) size 0x0 [color=#0000EE]
              RenderText {#text} at (0,0) size 0x0
          RenderBlock (anonymous) at (0,0) size 744x18 [color=#0000EE]
            RenderBlock {SPAN} at (0,0) size 744x18
              RenderListMarker at (-20,0) size 16x17 [color=#000000]: "1"
              RenderText {#text} at (0,0) size 96x17
                text run at (0,0) width 96: "first block span"
          RenderBlock (anonymous) at (0,18) size 744x18
            RenderInline {A} at (0,0) size 68x17 [color=#0000EE]
              RenderInline {SPAN} at (0,0) size 68x17
                RenderText {#text} at (0,0) size 68x17
                  text run at (0,0) width 68: "inline span"
              RenderText {#text} at (0,0) size 0x0
          RenderBlock (anonymous) at (0,36) size 744x18 [color=#0000EE]
            RenderBlock {SPAN} at (0,0) size 744x18
              RenderText {#text} at (0,0) size 116x17
                text run at (0,0) width 116: "second block span"
          RenderBlock (anonymous) at (0,54) size 744x0
            RenderInline {A} at (0,0) size 0x0 [color=#0000EE]
            RenderText {#text} at (0,0) size 0x0

So, the RenderListMarker is a child of RenderBlock {SPAN}, and not RenderListItem {LI}. In this case the marker is ignored and not exposed to ATs.
Comment 1 Radar WebKit Bug Importer 2022-02-17 05:53:56 PST
<rdar://problem/89082485>
Comment 2 Carlos Garcia Campos 2022-02-17 05:58:35 PST
Created attachment 452357 [details]
Patch
Comment 3 Andres Gonzalez 2022-02-17 06:41:45 PST
(In reply to Carlos Garcia Campos from comment #2)
> Created attachment 452357 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -512,7 +512,15 @@ AccessibilityObject* AccessibilityRenderObject::parentObject() const

+    if (m_renderer->isListMarker()) {
+        if (auto* listItem = ancestorsOfType<RenderListItem>(*m_renderer).first()) {
+            AccessibilityObject* parent = axObjectCache()->getOrCreate(listItem);
+            if (parent->isListItem())
+                return parent;
+        }
+    }
+

Creating new objects here is problematic for isolated tree mode. Not sure it makes sense at all to create a new object when we are getting its parent. Perhaps 

auto* parent = axObjectCache()->get(listItem);

instead?

Also note that below that block we are getting the cache again:

     AXObjectCache* cache = axObjectCache();
     if (!cache)

Can we avoid the code duplication?

-    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj = obj->nextSibling())
-        addChild(obj.get());
+    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj = obj->nextSibling()) {
+        if (!obj->renderer()->isListMarker())
+            addChild(obj.get());
+    }

Since you are touching this code, could you please fix the style here? Use auto in the for statement, change obj -> object, ...

Can obj->renderer() be null?

I wonder if it would be better to special case the list items in this loop and add the markers just for them.
Comment 4 Andres Gonzalez 2022-02-17 06:44:38 PST
Is this case covered in the Mac and iOS platforms?
Comment 5 Andres Gonzalez 2022-02-17 06:45:32 PST
I meant test case.
Comment 6 chris fleizach 2022-02-17 14:06:15 PST
Comment on attachment 452357 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3294
> +        if (!obj->renderer()->isListMarker())

should we put this logic in addChild() ?
Comment 7 Carlos Garcia Campos 2022-02-18 01:11:25 PST
(In reply to Andres Gonzalez from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Created attachment 452357 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> @@ -512,7 +512,15 @@ AccessibilityObject*
> AccessibilityRenderObject::parentObject() const
> 
> +    if (m_renderer->isListMarker()) {
> +        if (auto* listItem =
> ancestorsOfType<RenderListItem>(*m_renderer).first()) {
> +            AccessibilityObject* parent =
> axObjectCache()->getOrCreate(listItem);
> +            if (parent->isListItem())
> +                return parent;
> +        }
> +    }
> +
> 
> Creating new objects here is problematic for isolated tree mode. Not sure it
> makes sense at all to create a new object when we are getting its parent.

We do the same for Menu and MenuBar exceptions, I followed the existing approach. And the non-exceptions path also uses getOrCreate() for the renderParentObject().

> Perhaps 
> 
> auto* parent = axObjectCache()->get(listItem);
> 
> instead?
> 
> Also note that below that block we are getting the cache again:
> 
>      AXObjectCache* cache = axObjectCache();
>      if (!cache)
> 
> Can we avoid the code duplication?

Yes, we should handle the cache being null in all the cases, also for the Menu and MenuBar exceptions.

> -    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj =
> obj->nextSibling())
> -        addChild(obj.get());
> +    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj =
> obj->nextSibling()) {
> +        if (!obj->renderer()->isListMarker())
> +            addChild(obj.get());
> +    }
> 
> Since you are touching this code, could you please fix the style here? Use
> auto in the for statement, change obj -> object, ...

Sure.

> Can obj->renderer() be null?

No, firstChild() and nextSibling() return nullptr early if renderer is null.

> I wonder if it would be better to special case the list items in this loop
> and add the markers just for them.

I think we always want the marker to be the first child of a list item, that's why I added it after all other children are already added.
Comment 8 Carlos Garcia Campos 2022-02-18 01:12:26 PST
(In reply to Andres Gonzalez from comment #4)
> Is this case covered in the Mac and iOS platforms?

It seems so, because there are tests failing. I'll check the failures, and maybe it's a good idea to add a specific test for this particular case.
Comment 9 Carlos Garcia Campos 2022-02-18 01:17:03 PST
(In reply to chris fleizach from comment #6)
> Comment on attachment 452357 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452357&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3294
> > +        if (!obj->renderer()->isListMarker())
> 
> should we put this logic in addChild() ?

I guess we could override addChild and move the check there, but we don't really need the check in all other calls to addChild in AccessibilityRenderObject
Comment 10 Carlos Garcia Campos 2022-02-18 01:29:10 PST
Checking the failure of accessibility/mac/attributed-string-with-listitem-multiple-lines.html I wonder if mac really wants this behavior. What's the difference between AXListMarker and AXTextMarkerRangeForUIElement? Does mac want to expose the list item marker as the former or the latter?
Comment 11 Carlos Garcia Campos 2022-02-18 03:39:16 PST
(In reply to Andres Gonzalez from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Created attachment 452357 [details]
> > Patch
[...]
> 
> -    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj =
> obj->nextSibling())
> -        addChild(obj.get());
> +    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj =
> obj->nextSibling()) {
> +        if (!obj->renderer()->isListMarker())
> +            addChild(obj.get());
> +    }
> 
> Since you are touching this code, could you please fix the style here? Use
> auto in the for statement, change obj -> object, ...

Note that auto can't be used here because firstChild() and nextSibling() return a raw pointer. I guess we are taking a reference for a good reason, so I won't change that, at least not in this patch.
Comment 12 Carlos Garcia Campos 2022-02-18 03:50:49 PST
Created attachment 452505 [details]
Patch

Not asking r? because mac tests will still fail, but it should address review comments and fix the GTK layout test failure. To fix the apple tests I need to understand the expected behavior because maybe we only want this change to ATSPI.
Comment 13 Carlos Garcia Campos 2022-02-23 00:55:39 PST
Created attachment 452948 [details]
Patch

I'm making this patch atspi specific, because I don't know what the expected behavior in mac is.
Comment 14 Carlos Garcia Campos 2022-02-24 01:27:23 PST
Created attachment 453083 [details]
Patch
Comment 15 Carlos Garcia Campos 2022-02-25 01:17:50 PST
Committed r290502 (247789@trunk): <https://commits.webkit.org/247789@trunk>
Comment 16 Kimmo Kinnunen 2022-02-25 04:38:35 PST
Reopening to attach new patch.
Comment 17 Kimmo Kinnunen 2022-02-25 04:38:39 PST
Created attachment 453195 [details]
Patch for landing