ATK 2.11.90 introduces AtkTableCell interface so that AccessibilityTableCell could be directly exposed to ATs, at least some of its functionality.
<rdar://problem/16145947>
Created attachment 225244 [details] patch
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.
(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.
(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.
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"
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.
(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.
(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
Created attachment 225260 [details] patch
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.
(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.
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
(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.
Comment on attachment 225260 [details] patch Clearing flags on attachment: 225260 Committed r164786: <http://trac.webkit.org/changeset/164786>
All reviewed patches have been landed. Closing bug.