Summary: | AX: List item marker not exposed when not a direct child of a list item | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | Accessibility | Assignee: | 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
Carlos Garcia Campos
2022-02-17 05:53:46 PST
Created attachment 452357 [details]
Patch
(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. Is this case covered in the Mac and iOS platforms? I meant test case. 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() ? (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. (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. (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 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? (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. 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.
Created attachment 452948 [details]
Patch
I'm making this patch atspi specific, because I don't know what the expected behavior in mac is.
Created attachment 453083 [details]
Patch
Committed r290502 (247789@trunk): <https://commits.webkit.org/247789@trunk> Reopening to attach new patch. Created attachment 453195 [details]
Patch for landing
|