[Webkit-unassigned] [Bug 260621] AX: aria-controls-with-tabs fails in ITM

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 23 12:04:18 PDT 2023


Tyler Wilcock <tyler_w at apple.com> changed:

           What    |Removed                     |Added
                 CC|                            |tyler_w at apple.com

--- Comment #5 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 467398
  --> https://bugs.webkit.org/attachment.cgi?id=467398

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1542
> +    RefPtr<AccessibilityObject> object = getOrCreate(node);

I think this can just be RefPtr object = ... (no <> necessary)

> Source/WebCore/accessibility/AXObjectCache.cpp:1544
> +    if (object->isTabItem()) {

Do we need to null-check `object` before dereferencing here?

> Source/WebCore/accessibility/AXObjectCache.cpp:1558
> +    auto controllers = focusedControlledPanelItem->controllers();
> +
> +    for (auto& axObject : controllers)

Optional nit: this newline feels unnecessary to me.

> Source/WebCore/accessibility/AXObjectCache.cpp:1645
> +    // FIXME: optimize with ancestor flags to determine if an element is in a tabpanel

I feel like this could be written more clearly for future us...isn't the idea more like "FIXME: Consider creating a new ancestor flag to only do this work when |oldNode| or |newNode| have a tab panel ancestor (as that's the only time it is necessary)"? Or is that not accurate?

> Source/WebCore/accessibility/AXObjectCache.cpp:1685
>      else if (is<HTMLOptionElement>(node))
>          postNotification(node, AXSelectedStateChanged);
> +    else if (nodeHasRole(node, "tab"_s))
> +        postNotification(node, AXSelectedStateChanged);

Can we combine this with the previous else if rather than adding a new case?

else if (is<HTMLOptionElement>(node) || nodeHasRole(node, "tab"_s))

> Source/WebCore/accessibility/AXObjectCache.h:570
> +    void handleTabOrControlledTabPanelSelected(Node*);

Any time we add a method here, we need to make sure we add a no-op method to the !ENABLE(ACCESSIBILITY) section of this file to avoid breaking the Playstation build. See `handleRoleChanged` as an example.

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20230823/0c37f949/attachment.htm>

More information about the webkit-unassigned mailing list