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

Carlos Garcia Campos
Reported 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.
Attachments
Patch (10.47 KB, patch)
2022-02-17 05:58 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Patch (11.58 KB, patch)
2022-02-18 03:50 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Patch (11.75 KB, patch)
2022-02-23 00:55 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Patch (11.87 KB, patch)
2022-02-24 01:27 PST, Carlos Garcia Campos
no flags
Patch for landing (12.56 KB, patch)
2022-02-25 04:38 PST, Kimmo Kinnunen
no flags
Radar WebKit Bug Importer
Comment 1 2022-02-17 05:53:56 PST
Carlos Garcia Campos
Comment 2 2022-02-17 05:58:35 PST
Andres Gonzalez
Comment 3 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.
Andres Gonzalez
Comment 4 2022-02-17 06:44:38 PST
Is this case covered in the Mac and iOS platforms?
Andres Gonzalez
Comment 5 2022-02-17 06:45:32 PST
I meant test case.
chris fleizach
Comment 6 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() ?
Carlos Garcia Campos
Comment 7 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.
Carlos Garcia Campos
Comment 8 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.
Carlos Garcia Campos
Comment 9 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
Carlos Garcia Campos
Comment 10 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?
Carlos Garcia Campos
Comment 11 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.
Carlos Garcia Campos
Comment 12 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.
Carlos Garcia Campos
Comment 13 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.
Carlos Garcia Campos
Comment 14 2022-02-24 01:27:23 PST
Carlos Garcia Campos
Comment 15 2022-02-25 01:17:50 PST
Kimmo Kinnunen
Comment 16 2022-02-25 04:38:35 PST
Reopening to attach new patch.
Kimmo Kinnunen
Comment 17 2022-02-25 04:38:39 PST
Created attachment 453195 [details] Patch for landing
Note You need to log in before you can comment on or make changes to this bug.