Bug 118179

Summary: [ATK] Refactor code for translating offsets between WebCore a11y and ATK
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Mario Sanchez Prada <mario>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, commit-queue, dmazzoni, jdiggs, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 114871    
Attachments:
Description Flags
Patch proposal
none
Patch proposal
none
Patch proposal
none
Patch proposal cfleizach: review+

Description Mario Sanchez Prada 2013-06-28 03:07:21 PDT
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.
Comment 1 Mario Sanchez Prada 2013-06-28 05:19:51 PDT
Created attachment 205700 [details]
Patch proposal

Here comes the patch
Comment 2 chris fleizach 2013-06-28 11:47:44 PDT
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
Comment 3 Chris Dumez 2013-06-30 23:40:04 PDT
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].
Comment 4 Mario Sanchez Prada 2013-07-01 03:41:12 PDT
(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.
Comment 5 Mario Sanchez Prada 2013-07-02 05:54:34 PDT
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.
Comment 6 Mario Sanchez Prada 2013-07-10 09:49:41 PDT
(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? :)
Comment 7 chris fleizach 2013-07-10 10:28:44 PDT
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?
Comment 8 Mario Sanchez Prada 2013-07-10 17:47:38 PDT
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.
Comment 9 chris fleizach 2013-07-10 17:50:45 PDT
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
Comment 10 Mario Sanchez Prada 2013-07-10 18:19:22 PDT
Created attachment 206420 [details]
Patch proposal

Attaching new patch with the function name changed
Comment 11 chris fleizach 2013-07-10 21:16:47 PDT
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
Comment 12 Mario Sanchez Prada 2013-07-11 00:41:56 PDT
(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.
Comment 13 Mario Sanchez Prada 2013-07-11 00:52:02 PDT
Created attachment 206431 [details]
Patch proposal

Attaching new patch with the ChangeLog updated
Comment 14 chris fleizach 2013-07-11 00:55:37 PDT
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
Comment 15 Mario Sanchez Prada 2013-07-11 00:57:38 PDT
(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
Comment 16 Mario Sanchez Prada 2013-07-11 01:37:15 PDT
Committed r152561: <http://trac.webkit.org/changeset/152561>