Bug 236777 – AX: List item marker not exposed when not a direct child of a list item

• Component: Accessibility
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Radar WebKit Bug Importer]

<rdar://problem/89082485>

[Carlos Garcia Campos]

Created attachment 452357 [details]
Patch

[Andres Gonzalez]

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

[Andres Gonzalez]

Is this case covered in the Mac and iOS platforms?

[Andres Gonzalez]

I meant test case.

[chris fleizach]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

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?

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 452948 [details]
Patch

I'm making this patch atspi specific, because I don't know what the expected behavior in mac is.

[Carlos Garcia Campos]

Created attachment 453083 [details]
Patch

[Carlos Garcia Campos]

Committed r290502 (247789@trunk): <https://commits.webkit.org/247789@trunk>

[Kimmo Kinnunen]

Reopening to attach new patch.

[Kimmo Kinnunen]

Created attachment 453195 [details]
Patch for landing