[webkit-reviews] review canceled: [Bug 72811] [Gtk] No accessible caret-moved events found in certain content : [Attachment 160678] proposed fix - part 2 (addressed feedback from review)

[bugzilla-daemon at webkit.org]

Joanmarie Diggs <jdiggs at igalia.com> has canceled Joanmarie Diggs
<jdiggs at igalia.com>'s request for review:
Bug 72811: [Gtk] No accessible caret-moved events found in certain content
https://bugs.webkit.org/show_bug.cgi?id=72811

Attachment 160678: proposed fix - part 2 (addressed feedback from review)
https://bugs.webkit.org/attachment.cgi?id=160678&action=review

------- Additional Comments from Joanmarie Diggs <jdiggs at igalia.com>
(In reply to comment #35)

Thanks Chris! I fear this time I may have gone too far in the other direction
(too long a comment), but I hope not.
 
> If I had to guess it's because 
> 
> "<span> tags are inline tags and not meant to convey information if they have
no other aria information on them. If we don't ignore them, then they can cause
other elements to be ignored because...."

(They also cause events to be emitted from the wrong object.)

Both explained.

> > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:83
> > +	     && parent->ariaRoleAttribute() == UnknownRole)
> 
> this seems like very specific logic. Generally we don't make inclusion
decisions based on what the parent is doing. can you add a better comment here
why an anonymous block, who's parent is not the body, and who's parent is not
an unknown role, is ignored?
> 
> also, why does it only matter for the parent's aria role but not it's normal
role?

Bottom line is being conservative and going with the default behavior for now.
But I detailed the areas.
 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:822
> > +	 return role == ParagraphRole || role == HeadingRole || role == DivRole
|| role == CellRole || role == ListItemRole;
> 
> this patch here seems like a separate issue from the rest of this patch

It does seem so on the surface. But it turns out that without that change, this
patch introduces a regression in one of the unit tests. As I mentioned in
comment 15, while list item is an always-expect-AtkText role I had not
explicitly included it because we already had role-specific code handling that
situation. Turned out the handling was based on an unwanted, unexpected
GroupRole object. That is my mistake, for which I apologize.