Bug 120456

Summary: [AX][ATK] Added support for sort and help attributes.
Product: WebKit Reporter: Krzysztof Czech <k.czech>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jdiggs, mario, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
cfleizach: review+, commit-queue: commit-queue-
Patch none

Description Krzysztof Czech 2013-08-29 01:53:50 PDT
Added support for aria-help and aria-sort attributes.
Sharing aria-sort.html specific mac test with other ports.
Comment 1 Radar WebKit Bug Importer 2013-08-29 01:54:11 PDT
<rdar://problem/14866030>
Comment 2 Krzysztof Czech 2013-08-29 02:07:12 PDT
Created attachment 209960 [details]
Patch
Comment 3 WebKit Commit Bot 2013-08-29 02:08:36 PDT
Attachment 209960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/aria-sort-expected.txt', u'LayoutTests/accessibility/aria-sort.html', u'LayoutTests/platform/efl-wk2/TestExpectations', u'LayoutTests/platform/efl-wk2/accessibility/image-link-expected.txt', u'LayoutTests/platform/efl-wk2/accessibility/image-map2-expected.txt', u'LayoutTests/platform/efl-wk2/accessibility/table-cell-spans-expected.txt', u'LayoutTests/platform/efl-wk2/accessibility/table-cells-expected.txt', u'LayoutTests/platform/gtk/accessibility/image-link-expected.txt', u'LayoutTests/platform/gtk/accessibility/image-map2-expected.txt', u'LayoutTests/platform/gtk/accessibility/table-cell-spans-expected.txt', u'LayoutTests/platform/gtk/accessibility/table-cells-expected.txt', u'LayoutTests/platform/mac/accessibility/aria-sort-expected.txt', u'LayoutTests/platform/mac/accessibility/aria-sort.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:55:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Krzysztof Czech 2013-08-29 02:24:00 PDT
Created attachment 209961 [details]
Patch
Comment 5 chris fleizach 2013-08-29 09:03:44 PDT
Comment on attachment 209961 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +              accessibility/aria-help.html

the change log says aria-help is added, but i don't see it in this patch

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:54
> +    return attributeString == "AXSortDirection" ? "aria-sort" : String();

since it seems like you'll continue adding to this method you should probably add sort direction to the other list of if statements directly above, and then leave the return "" as the final line

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:58
> +    return attributeString == "AXSortDirection" ? "aria-sort" : String();

ditto with comment from DRT
Comment 6 Krzysztof Czech 2013-08-30 01:21:01 PDT
Created attachment 210076 [details]
Patch
Comment 7 Krzysztof Czech 2013-08-30 01:23:17 PDT
> > Source/WebCore/ChangeLog:11
> > +              accessibility/aria-help.html
> 
> the change log says aria-help is added, but i don't see it in this patch
Yes right, I did not catch this.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:54
> > +    return attributeString == "AXSortDirection" ? "aria-sort" : String();
> 
> since it seems like you'll continue adding to this method you should probably add sort direction to the other list of if statements directly above, and then leave the return "" as the final line
Yes, it seems other attributes will be added. Sounds good.
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:58
> > +    return attributeString == "AXSortDirection" ? "aria-sort" : String();
> 
> ditto with comment from DRT
Sounds good.
Comment 8 WebKit Commit Bot 2013-09-02 09:25:26 PDT
Comment on attachment 210076 [details]
Patch

Rejecting attachment 210076 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 210076, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 file Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp.rej
patching file Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp
Hunk #1 FAILED at 52.
Hunk #2 succeeded at 698 (offset 6 lines).
1 out of 2 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Fleizach']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/1603987
Comment 9 Krzysztof Czech 2013-09-03 03:14:25 PDT
Created attachment 210345 [details]
Patch
Comment 10 WebKit Commit Bot 2013-09-03 04:28:30 PDT
Comment on attachment 210345 [details]
Patch

Clearing flags on attachment: 210345

Committed r154976: <http://trac.webkit.org/changeset/154976>
Comment 11 WebKit Commit Bot 2013-09-03 04:28:33 PDT
All reviewed patches have been landed.  Closing bug.