[Webkit-unassigned] [Bug 169924] AX: WebKit should not expose redundant AXGroups with missing role when the label is the same as the contents

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=169924

--- Comment #9 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to Tyler Wilcock from comment #8)
> > +                if (nodeDocument->childNeedsStyleRecalc())
> > +                    return String();
> >
> > return { };
> 
> There is code two lines above that also does `return String();`. I can
> change both if that's the style preference here.

Yes, feel free to change the other two. We don't do mass style changes but if you are adding new code or touching a function, you may just fix it up.

> 
> > +    // Never ignore a <div> with event listeners attached to it (e.g.
> > onclick).
> > +    if (axObject.node() && axObject.node()->hasEventListeners())
> > +        return false;
> > 
> > Isn't this check done for any object already in the isIgnored method?
> > Wondering why we need to do this specifically for groups.
> 
> If it is, I can't find it. It probably should exist everywhere?
> 
> Regardless, I think we'd still need this check to avoid returning a false
> positive for a group with redundant AX text and an event handler.
> 
> > +    auto* first = axObject.firstChild();
> > +    if (first && first == axObject.lastChild() && first->roleValue() ==
> > AccessibilityRole::StaticText) {
> > 
> > Wouldn't this be more efficient?
> > 
> > +    auto& children = axObject.children();
> > +    if (children.size() == 1 && children[0]->roleValue() ==
> > AccessibilityRole::StaticText) {
> > 
> > or the problem is that this is used in children()?
> 
> We can't call children() here because children() is not const (it updates
> the children if required). Making any "is ignored" function non-const will
> make many other things have to be non-const as well.
> 
> This is generally unfortunate, because it makes ignoring things based on
> their contents much more difficult. We'll have to comb through the AX
> objects represented by the raw layout tree ourselves. It works fine in this
> simple example, but more complex analyses will be harder and fragile.

Maybe we need a variant of children that doesn't do an update. And  we need to rework the first and last child two. But that is for a separate patch.

> 
> > +        Vector<AccessibilityText> text;
> > ...
> > +        auto axText = text.size() ? text[0].text : String();
> > 
> > Aren't these names inverted?  i.e., shouldn't the String be text and the
> > vector axText?
> > 
> > What if the vector has more than one element? Perhaps we should return false
> > in that case.
> 
> My understanding of accessibilityText is that it's ordered in terms of the
> most applicable text based on https://www.w3.org/TR/accname-1.2/#step1. So
> by picking the first one, we are choosing the one AX clients should be using
> when describing the element. Web Inspector also picks the first
> accessibilityText.

-- 
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/20211014/a85dcb25/attachment-0001.htm>