Bug 129250 – [ATK] Utilize AtkTableCell to expose directly AccessibilityTableCell to AT.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Krzysztof Czech]

ATK 2.11.90 introduces AtkTableCell interface so that AccessibilityTableCell could be directly exposed to ATs, at least some of its functionality.

[Radar WebKit Bug Importer]

<rdar://problem/16145947>

[Krzysztof Czech]

Created attachment 225244 [details]
patch

[WebKit Commit Bot]

Attachment 225244 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1075:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #2)
> Created an attachment (id=225244) [details]
> patch
Patch also introduces implementation of methods for getting column/row headers represented as an array of cells.

[Krzysztof Czech]

(In reply to comment #3)
> Attachment 225244 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1075:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 1 in 15 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is something that is here for a while and I guess it could be changed (or not) in a separate patch.

[Mario Sanchez Prada]

Comment on attachment 225244 [details]
patch

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

Looks mostly good to me. I've got just a few comments before the r+

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:57
> +    if (!axObject || !axObject->isAccessibilityRenderObject())

You should check isAccessibilityTableCell here, instead of isAccessibilityRenderObject

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:72
> +    if (!axObject || !axObject->isAccessibilityRenderObject())

Ditto.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:488
> +static void convertGPtrArrayToVector(const GPtrArray* array, Vector<AccessibilityUIElement>& elements)

This function is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:38
> +#include <WebKit2/WKBundleFrame.h>

This include is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:554
> +static Vector<RefPtr<AccessibilityUIElement>> convertGPtrArrayToVector(const GPtrArray* array)

This function is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:564
> +static JSValueRef convertToJSObjectArray(const Vector<RefPtr<AccessibilityUIElement>>& children)

Ditto

> Tools/efl/jhbuild.modules:344
> +    <branch module="pub/GNOME/sources/atk/2.11/atk-2.11.90.tar.xz" version="2.11.9"

version should be "2.11.90"

[Alejandro Piñeiro]

First, thanks for working so quick on implementing this interface, officially you are the first ones implementing this interface. You even started before the new API was included on a stable release.

As I said, this is the first implementation of the interface. It is really usual that when you start to implement a new interface, you find some kind of problems. So if that is the case, and you think that there is some problem with the new API, don't hesitate to comment that. Solving those issues now, when nobody else has implemented it yet, would be easier that in the future. In fact, as this new API was not included on a stable release, it could be possible to change the API without letting deprecated methods around.

And talking about deprecated methods. For your information, as a collateral effect of this new interface, some AtkTable methods were deprecated (like atk_table_get_index_at). Just in case you want to take that into consideration.

Again thanks.

[Krzysztof Czech]

(In reply to comment #7)
> First, thanks for working so quick on implementing this interface, officially you are the first ones implementing this interface. You even started before the new API was included on a stable release.
Do you suggest to a wait a bit till the stable release comes out ?.
> 
> As I said, this is the first implementation of the interface. It is really usual that when you start to implement a new interface, you find some kind of problems. So if that is the case, and you think that there is some problem with the new API, don't hesitate to comment that. Solving those issues now, when nobody else has implemented it yet, would be easier that in the future. In fact, as this new API was not included on a stable release, it could be possible to change the API without letting deprecated methods around.
Thanks, frankly speaking everything worked as expected. I did not have any problems when adapting this interface to the WebKit's world at least when implementing get_row_header_cells and get_column_header_cells. If find some issues I will comment that.
> 
> And talking about deprecated methods. For your information, as a collateral effect of this new interface, some AtkTable methods were deprecated (like atk_table_get_index_at). Just in case you want to take that into consideration.
Yes, I've noticed that, I will try to adapt current WebKit's implementation when possible.
> 
> Again thanks.

[Alejandro Piñeiro]

(In reply to comment #8)
> (In reply to comment #7)
> > First, thanks for working so quick on implementing this interface, officially you are the first ones implementing this interface. You even started before the new API was included on a stable release.
> Do you suggest to a wait a bit till the stable release comes out ?.

Oh no, it was a compliment. The earlier someone (in this case you, and thanks) start to do a implementation of the interface on a real use case the better. As I said, that could help us to detect early bugs, and to improve it based on the experience.

> > As I said, this is the first implementation of the interface. It is really usual that when you start to implement a new interface, you find some kind of problems. So if that is the case, and you think that there is some problem with the new API, don't hesitate to comment that. Solving those issues now, when nobody else has implemented it yet, would be easier that in the future. In fact, as this new API was not included on a stable release, it could be possible to change the API without letting deprecated methods around.
> Thanks, frankly speaking everything worked as expected. I did not have any problems when adapting this interface to the WebKit's world at least when implementing get_row_header_cells and get_column_header_cells. If find some issues I will comment that.
> > 
> > And talking about deprecated methods. For your information, as a collateral effect of this new interface, some AtkTable methods were deprecated (like atk_table_get_index_at). Just in case you want to take that into consideration.
> Yes, I've noticed that, I will try to adapt current WebKit's implementation when possible.
> > 
> > Again thanks.

Ok

[Krzysztof Czech]

Created attachment 225260 [details]
patch

[WebKit Commit Bot]

Attachment 225260 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1075:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #6)
> (From update of attachment 225244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225244&action=review
> 
> Looks mostly good to me. I've got just a few comments before the r+
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:57
> > +    if (!axObject || !axObject->isAccessibilityRenderObject())
> 
> You should check isAccessibilityTableCell here, instead of isAccessibilityRenderObject
Done. I used isTableCell()
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:72
> > +    if (!axObject || !axObject->isAccessibilityRenderObject())
> 
> Ditto.
Done
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:488
> > +static void convertGPtrArrayToVector(const GPtrArray* array, Vector<AccessibilityUIElement>& elements)
> 
> This function is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards
Done
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:38
> > +#include <WebKit2/WKBundleFrame.h>
> 
> This include is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards
Done
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:554
> > +static Vector<RefPtr<AccessibilityUIElement>> convertGPtrArrayToVector(const GPtrArray* array)
> 
> This function is only used from the new code added below, so I think we should put it inside #if ATK_CHECK_VERSION(2,11,90) guards
Done
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:564
> > +static JSValueRef convertToJSObjectArray(const Vector<RefPtr<AccessibilityUIElement>>& children)
> 
> Ditto
Done
> 
> > Tools/efl/jhbuild.modules:344
> > +    <branch module="pub/GNOME/sources/atk/2.11/atk-2.11.90.tar.xz" version="2.11.9"
> 
> version should be "2.11.90"

Done

Thanks Mario.

[Mario Sanchez Prada]

Comment on attachment 225260 [details]
patch

Lgtm. About the style errors, I think we can/should fix that in a separate patch. It's an implementation file not exposed to GTK/EFL outside worlds at all, so it should be fine using things such as WAIEditableText or the like

[Krzysztof Czech]

(In reply to comment #13)
> (From update of attachment 225260 [details])
> Lgtm. About the style errors, I think we can/should fix that in a separate patch. It's an implementation file not exposed to GTK/EFL outside worlds at all, so it should be fine using things such as WAIEditableText or the like

Thanks Mario. This what I thought regarding style errors. I will fix this up in separate patch.

[WebKit Commit Bot]

Comment on attachment 225260 [details]
patch

Clearing flags on attachment: 225260

Committed r164786: <http://trac.webkit.org/changeset/164786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.