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.
Created attachment 205700 [details] Patch proposal Here comes the patch
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 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].
(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.
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.
(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 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 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 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
Created attachment 206420 [details] Patch proposal Attaching new patch with the function name changed
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
(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.
Created attachment 206431 [details] Patch proposal Attaching new patch with the ChangeLog updated
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
(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
Committed r152561: <http://trac.webkit.org/changeset/152561>