Bug 51859 - WK2: Support Accessibility
Summary: WK2: Support Accessibility
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-03 18:01 PST by chris fleizach
Modified: 2011-01-05 11:24 PST (History)
9 users (show)

See Also:


Attachments
Patch (102.29 KB, patch)
2011-01-04 10:26 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (112.03 KB, patch)
2011-01-04 10:52 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (120.50 KB, patch)
2011-01-04 12:17 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (122.92 KB, patch)
2011-01-04 12:59 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (123.46 KB, patch)
2011-01-04 14:14 PST, chris fleizach
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2011-01-03 18:01:58 PST
Need to support accessibility for the Mac
Comment 1 chris fleizach 2011-01-04 10:26:22 PST
Created attachment 77896 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-04 10:45:13 PST
Attachment 77896 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7314339
Comment 3 Early Warning System Bot 2011-01-04 10:49:58 PST
Attachment 77896 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7246433
Comment 4 chris fleizach 2011-01-04 10:52:31 PST
Created attachment 77907 [details]
Patch
Comment 5 Build Bot 2011-01-04 11:16:39 PST
Attachment 77896 [details] did not build on win:
Build output: http://queues.webkit.org/results/7338296
Comment 6 Early Warning System Bot 2011-01-04 11:42:20 PST
Attachment 77907 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7327339
Comment 7 chris fleizach 2011-01-04 12:17:33 PST
Created attachment 77914 [details]
Patch
Comment 8 Build Bot 2011-01-04 12:26:25 PST
Attachment 77907 [details] did not build on win:
Build output: http://queues.webkit.org/results/7243408
Comment 9 Early Warning System Bot 2011-01-04 12:31:37 PST
Attachment 77914 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7201407
Comment 10 Build Bot 2011-01-04 12:48:12 PST
Attachment 77914 [details] did not build on win:
Build output: http://queues.webkit.org/results/7235431
Comment 11 chris fleizach 2011-01-04 12:59:05 PST
Created attachment 77921 [details]
Patch
Comment 12 Build Bot 2011-01-04 13:52:26 PST
Attachment 77921 [details] did not build on win:
Build output: http://queues.webkit.org/results/7288367
Comment 13 chris fleizach 2011-01-04 14:14:05 PST
Created attachment 77929 [details]
Patch
Comment 14 Sam Weinig 2011-01-04 15:49:28 PST
Comment on attachment 77929 [details]
Patch

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

This overall looks really good. Maybe Anders can give it a once over as well.

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:31
> +#import <AppKit/NSColor.h>

What is this for?  It doesn't seem related.

> WebCore/accessibility/AccessibilityScrollView.cpp:127
> +}
> +
> +    
> +AccessibilityObject* AccessibilityScrollView::accessibilityHitTest(const IntPoint& point) const

Extra space.

> WebCore/accessibility/AccessibilityScrollbar.cpp:39
> +AccessibilityScrollbar::AccessibilityScrollbar(Scrollbar *scrollbar)

* on the wrong side.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1027
>  
> +    else if (m_object->isScrollView())
> +        objectAttributes = scrollViewAttrs;

The newline seems unnecessary.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1125
> +    }
> +    else {

The } should be on the same line as the else.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1131
> +            point = [[view window] convertBaseToScreen: [view convertPoint: point toView:nil]];

We usually don't put a space after the :.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1459
> +    if (scroll->platformWidget())
> +        return scroll->platformWidget();
> +    else
> +        return [self remoteAccessibilityParentObject];

The else is not necessary here.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1730
>          }
> +        else if (m_object->isScrollView()) {
> +            AccessibilityObject::AccessibilityChildrenVector children = m_object->children();

} should be on the same line as the else if.
Comment 15 chris fleizach 2011-01-04 15:52:06 PST
(In reply to comment #14)
> (From update of attachment 77929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77929&action=review
> 
> This overall looks really good. Maybe Anders can give it a once over as well.
> 
> > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:31
> > +#import <AppKit/NSColor.h>
> 
> What is this for?  It doesn't seem related.
> 

AccessibilityControllerMac.mm now includes WebFramePrivate instead of Internal. WebFramePrivate has a NSColor* in its header.

suggestions for what is expected here are welcome

will do everything else. thanks
Comment 16 chris fleizach 2011-01-04 17:08:35 PST
http://trac.webkit.org/changeset/75031
Comment 17 WebKit Review Bot 2011-01-04 17:19:57 PST
http://trac.webkit.org/changeset/75031 might have broken SnowLeopard Intel Release (Build)
Comment 18 chris fleizach 2011-01-04 17:22:57 PST
http://trac.webkit.org/changeset/75033
Comment 19 chris fleizach 2011-01-04 17:37:23 PST
http://trac.webkit.org/changeset/75034
Comment 20 chris fleizach 2011-01-04 18:09:56 PST
I created https://bugs.webkit.org/show_bug.cgi?id=51911 to handle the GTK layout test failure and other GTK related issues
Comment 21 chris fleizach 2011-01-04 18:20:47 PST
I made https://bugs.webkit.org/show_bug.cgi?id=51912 to handle the failing Windows tests
Comment 22 WebKit Review Bot 2011-01-04 18:30:02 PST
http://trac.webkit.org/changeset/75033 might have broken Leopard Intel Debug (Tests)
Comment 23 WebKit Review Bot 2011-01-04 18:30:09 PST
http://trac.webkit.org/changeset/75034 might have broken Leopard Intel Debug (Tests)
Comment 24 WebKit Review Bot 2011-01-05 01:59:29 PST
http://trac.webkit.org/changeset/75052 might have broken Leopard Intel Debug (Tests)
Comment 25 Ryosuke Niwa 2011-01-05 11:20:28 PST
http://trac.webkit.org/changeset/75031 seems to broke WIn7 accessibility tests:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75022%20(7918)/
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75031%20(7919)/

--- /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/platform/win/accessibility/document-role-expected.txt	2011-01-05 00:33:06.263629600 -0800
+++ /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/platform/win/accessibility/document-role-actual.txt	2011-01-05 00:33:06.261629400 -0800
@@ -1,4 +1,4 @@
 Test for role attribute of document 
 ----------------------------------- 
-role of Document : document 
+role of Document : client 

--- /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/platform/win/accessibility/selection-and-focus-expected.txt	2011-01-05 00:33:07.034673700 -0800
+++ /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/platform/win/accessibility/selection-and-focus-actual.txt	2011-01-05 00:33:07.032673500 -0800
@@ -2,7 +2,7 @@
 This tests that we can adjust focus and selection via accessibility API.
 
 
-PASS accessibilityController.focusedElement.role is "document"
+FAIL accessibilityController.focusedElement.role should be document. Was client.
 PASS accessibilityController.focusedElement.role is "list"
 
 Option 0 is selected.
Comment 26 Ryosuke Niwa 2011-01-05 11:24:12 PST
Apparently, the regression is tracked by https://bugs.webkit.org/show_bug.cgi?id=51912.