[webkit-reviews] review denied: [Bug 76069] [GTK] ATK text-caret-moved and text-selection-changed events not being emitted : [Attachment 122637] Patch proposal

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 76069: [GTK] ATK text-caret-moved and text-selection-changed events not
being emitted
https://bugs.webkit.org/show_bug.cgi?id=76069

Attachment 122637: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=122637&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122637&action=review


> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2745
> +    AccessibilityObject* actualObject = focusedObject;

Is the goal here to find the first unignored object among the focused object
and its parents? actualObject is a bad name, I think. Consider something like
firstUnignoredParentOfFocusedObject.

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2761
> +    // Make sure the reference object gets now updated if the focused
> +    // object was pointing to it, so we don't have the reference
> +    // object as a descendant of the actual object being considered.
> +    if (focusedObject != referenceObject)
> +	   referenceObject = actualObject;

This comment seems wrong: Should it say "Make sure the reference object is
updated if the focused object was *different?" Can't you do this check more
directly by just checking if referenceObject is a descendant of focusedObject
or actualObject?

You say in your comment " so we don't have the reference object as a descendant
of the actual object being considered," but this can still happen if
focusedObject == referenceObject and focusedObject is ignored in the a11y tree.
 I find this code, in general, pretty confusing.