Bug 118179 – [ATK] Refactor code for translating offsets between WebCore a11y and ATK

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

At the moment, and due to the extra flattening we do sometimes in WebKitGTK when it comes to exposing the accessibility hierarchy (e.g. list items and bullets get exposed as a single ATK Object) we need to do some adjustments whenever we need to consider offsets specified from the ATs (ATK world) and those handled internally in WebKit (WebCore a11y).

At the moment we have some snippets of code in the implementation of the AtkText interface, but it's sometimes not clear how to use them and so it would be good to refactor that code both to make it clearer and simpler.

[Mario Sanchez Prada]

Created attachment 205700 [details]
Patch proposal

Here comes the patch

[chris fleizach]

Comment on attachment 205700 [details]
Patch proposal

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

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:473
> +static int listItemMarkerLengthForObject(const AccessibilityObject* object)

this only applies to LTR, maybe the method should be named to indicate that?

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:475
> +    if (!object->isAccessibilityRenderObject())

you shouldn't have to check isAXRenderObject() -- renderer() is on every AXObject, so you just need to check if the renderer is 0 or not (which you're already doing)

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:482
> +        String markerText = toRenderListItem(renderer)->markerTextWithSuffix();

this can be made into one line of code

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:491
> +    if (!object->isAccessibilityRenderObject() || !object->isListItem())

AXrenderObject check is not necessary

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:500
> +    if (!coreObject || !coreObject->isAccessibilityRenderObject() || !coreObject->isListItem())

AXRenderObject check not necessary

[Chris Dumez]

Comment on attachment 205700 [details]
Patch proposal

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

> Source/WebCore/ChangeLog:3
> +        [GTK] Refactor code for translating offsets between WebCore a11y and ATK

This probably affects the EFL port as well so a [ATK] tag may be more appropriate than [GTK].

[Mario Sanchez Prada]

(In reply to comment #3)
> (From update of attachment 205700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205700&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [GTK] Refactor code for translating offsets between WebCore a11y and ATK
> 
> This probably affects the EFL port as well so a [ATK] tag may be more appropriate than [GTK].

Yes, you are right. Now changing the title and removing the Gtk keyword.

[Mario Sanchez Prada]

Created attachment 205897 [details]
Patch proposal

Thanks for the review, Chris. Now attaching a new patch addressing the issues you pointed out, see my comments below...

(In reply to comment #2)
> (From update of attachment 205700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205700&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:473
> > +static int listItemMarkerLengthForObject(const AccessibilityObject* object)
> 
> this only applies to LTR, maybe the method should be named to indicate that?

I already thought of that, but in the end I found it better, or at least more clear, not to do it since it's an internal matter from the point of view of that function. From the caller you just care about "normalizing" the offset for list items since it's an special element from the point of view of ATK (as we flatten it out together with the marker).

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:475
> > +    if (!object->isAccessibilityRenderObject())
> 
> you shouldn't have to check isAXRenderObject() -- renderer() is on every AXObject, so you just need to check if the renderer is 0 or not (which you're already doing)

Ok.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:482
> > +        String markerText = toRenderListItem(renderer)->markerTextWithSuffix();
> 
> this can be made into one line of code

Done.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:491
> > +    if (!object->isAccessibilityRenderObject() || !object->isListItem())
> 
> AXrenderObject check is not necessary

Ok.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:500
> > +    if (!coreObject || !coreObject->isAccessibilityRenderObject() || !coreObject->isListItem())
> 
> AXRenderObject check not necessary

Ok.

[Mario Sanchez Prada]

(In reply to comment #5)
> Created an attachment (id=205897) [details]
> Patch proposal
> 
> Thanks for the review, Chris. Now attaching a new patch addressing the issues
> you pointed out, see my comments below...

Ping reviewers? :)

[chris fleizach]

Comment on attachment 205897 [details]
Patch proposal

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

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:481
> +        return toRenderListItem(renderer)->markerTextWithSuffix().length();

So why does this method return 0 for listItemMarker when language is RTL?

[Mario Sanchez Prada]

Comment on attachment 205897 [details]
Patch proposal

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

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:481
>> +        return toRenderListItem(renderer)->markerTextWithSuffix().length();
> 
> So why does this method return 0 for listItemMarker when language is RTL?

This is because depending on the text direction we expose the marker before or after the actual text for the list item, to better match how it's visually represented on screen.

Thus, if it's RTL, the list item marker will be appended after the list item text in the exposed AtkObject which means we do not need to do any adjustment to the offsets when translating from Atk to WebCore or viceversa.

However, it's true that then it's not an appropriate name for this function. Perhaps it's better to go back to the original name as the one I used in the original patch from bug 114871: offsetAdjustmentForObject(), or offsetAdjustmentForListItem(). What do you think?

PS: In any case, we should maybe rethink this idea of appending/prepending the list item marker depending on text direction. I've checked what firefox does right now and they always prepend the list item marker when exposing AtkObjects for list items, regardless the text direction. In any case, that should be considered in a separate bug, if needed.

[chris fleizach]

Comment on attachment 205897 [details]
Patch proposal

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

>>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:481
>>> +        return toRenderListItem(renderer)->markerTextWithSuffix().length();
>> 
>> So why does this method return 0 for listItemMarker when language is RTL?
> 
> This is because depending on the text direction we expose the marker before or after the actual text for the list item, to better match how it's visually represented on screen.
> 
> Thus, if it's RTL, the list item marker will be appended after the list item text in the exposed AtkObject which means we do not need to do any adjustment to the offsets when translating from Atk to WebCore or viceversa.
> 
> However, it's true that then it's not an appropriate name for this function. Perhaps it's better to go back to the original name as the one I used in the original patch from bug 114871: offsetAdjustmentForObject(), or offsetAdjustmentForListItem(). What do you think?
> 
> PS: In any case, we should maybe rethink this idea of appending/prepending the list item marker depending on text direction. I've checked what firefox does right now and they always prepend the list item marker when exposing AtkObjects for list items, regardless the text direction. In any case, that should be considered in a separate bug, if needed.

offsetAdjustmentForListItem() is a good name i think

[Mario Sanchez Prada]

Created attachment 206420 [details]
Patch proposal

Attaching new patch with the function name changed

[chris fleizach]

Comment on attachment 206420 [details]
Patch proposal

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

there's also tests you're unskipping too right

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=118179

missing explanation of bug

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

fix this line

[Mario Sanchez Prada]

(In reply to comment #11)
> (From update of attachment 206420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206420&action=review
> 
> there's also tests you're unskipping too right

Not here, this is just a refactor to make clearer when we translate offsets from ATK to WebCore and the other way around, that I'm already using in further patches to remove the dependency of Pango.

> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=118179
> 
> missing explanation of bug

OOPS!. I'll add one

> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> fix this line

Sure.

[Mario Sanchez Prada]

Created attachment 206431 [details]
Patch proposal

Attaching new patch with the ChangeLog updated

[chris fleizach]

Comment on attachment 206431 [details]
Patch proposal

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

> Source/WebCore/ChangeLog:9
> +        between our ATK implementation and WebCore's accessiility layer.

misspelled:accessiility

[Mario Sanchez Prada]

(In reply to comment #14)
> (From update of attachment 206431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206431&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        between our ATK implementation and WebCore's accessiility layer.
> 
> misspelled:accessiility

Argh!! Anyway, thanks for the off-hours review. I'll fix that typo and push it as soon as I arrive in the office

[Mario Sanchez Prada]

Committed r152561: <http://trac.webkit.org/changeset/152561>