Bug 43005

Summary: AX: Support methods for web apps to interact with the native accessibility APIs
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, bdakin, ctguil, darin, ddkilzer, dglazkov, eric, gns, jcraig, ossy, sam, simon.fraser, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
cfleizach: review-, cfleizach: commit-queue-
patch
none
patch abarth: review-

Description chris fleizach 2010-07-26 14:39:26 PDT
With web apps becoming a popular option that can stand side-by-side with native apps, the need exists for web apps to be able to integrate with existing accessibility API more easily.

With that in mind, this patch extends Navigator to return an accessibility object. Within that is a screenReader object which can be queried.

Here is the initial API. Myself and members of the WAI-ARIA working group are working on a standard for this as well.


window.navigator.accessibility
… new accessibility object added as a property of window.navigator.

window.navigator.accessibility.screenChanged() 
… tells API entire screen has changed, update entire accessibility tree

window.navigator.accessibility.elementsChanged()
… tells API elements have changed and are finished changing for new (e.g. animation finished), assistive tech update any its interested in (like currently focused element)

window.navigator.accessibility.screenreader
… properties and methods specific to a screen reader. 

window.navigator.accessibility.screenreader.active
… true if the screen reader is running, false if not.

window.navigator.accessibility.screenreader.version
… string value similar to user agent string (e.g. 'Apple VoiceOver 3.0.1 (210.11)' )

window.navigator.accessibility. screenreader.activeElement
… VO cursor equivalent of document.activeElement for native kb focus
Comment 1 chris fleizach 2010-07-26 14:42:38 PDT
this initial patch will fail on a lot of systems i imagine. will gather build output to fix
Comment 2 chris fleizach 2010-07-26 14:45:35 PDT
Created attachment 62612 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2010-07-29 16:35:30 PDT
(In reply to comment #2)
> Created an attachment (id=62612) [details]
> Patch

Are you waiting for build output for this, or is the patch ready for review?

It looks like the bots all failed to apply the patch.
Comment 4 David Kilzer (:ddkilzer) 2010-07-29 16:36:02 PDT
Comment on attachment 62612 [details]
Patch

r- to post another patch for the bots.
Comment 5 chris fleizach 2010-07-29 16:50:38 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=62612) [details] [details]
> > Patch
> 
> Are you waiting for build output for this, or is the patch ready for review?
> 
> It looks like the bots all failed to apply the patch.

patch is ready, but i suspect it won't work on every build. will try again
Comment 6 chris fleizach 2010-07-30 09:57:11 PDT
Created attachment 63068 [details]
Patch
Comment 7 WebKit Review Bot 2010-07-30 10:02:39 PDT
Attachment 63068 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/page/Navigator.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/accessibility/AccessibilityAllInOne.cpp:29:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Early Warning System Bot 2010-07-30 10:06:39 PDT
Attachment 63068 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3610443
Comment 9 chris fleizach 2010-07-30 10:17:28 PDT
Created attachment 63074 [details]
Patch
Comment 10 WebKit Review Bot 2010-07-30 10:18:26 PDT
Attachment 63074 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/page/Navigator.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/accessibility/AccessibilityAllInOne.cpp:29:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Early Warning System Bot 2010-07-30 10:29:51 PDT
Attachment 63074 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3574705
Comment 12 chris fleizach 2010-07-30 10:41:35 PDT
Created attachment 63082 [details]
Patch
Comment 13 WebKit Review Bot 2010-07-30 10:43:37 PDT
Attachment 63082 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/DerivedSources.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/Navigator.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Review Bot 2010-07-30 10:45:49 PDT
Attachment 63068 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3590631
Comment 15 Early Warning System Bot 2010-07-30 10:59:44 PDT
Attachment 63082 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3633195
Comment 16 chris fleizach 2010-07-30 11:04:13 PDT
Created attachment 63086 [details]
Patch
Comment 17 WebKit Review Bot 2010-07-30 11:10:02 PDT
Attachment 63086 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/DerivedSources.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/Navigator.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Early Warning System Bot 2010-07-30 11:11:54 PDT
Attachment 63086 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3580603
Comment 19 WebKit Review Bot 2010-07-30 13:04:26 PDT
Attachment 63086 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3590635
Comment 20 WebKit Review Bot 2010-07-30 13:26:03 PDT
Attachment 63086 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3573632
Comment 21 chris fleizach 2010-07-30 14:25:59 PDT
Created attachment 63101 [details]
Patch
Comment 22 Early Warning System Bot 2010-07-30 14:33:16 PDT
Attachment 63101 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3634161
Comment 23 Eric Seidel (no email) 2010-07-30 14:34:19 PDT
Attachment 63101 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3561639
Comment 24 chris fleizach 2010-07-30 14:36:43 PDT
Created attachment 63102 [details]
Patch
Comment 25 Early Warning System Bot 2010-07-30 14:43:42 PDT
Attachment 63102 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3611423
Comment 26 WebKit Review Bot 2010-07-30 14:44:05 PDT
Attachment 63101 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3572654
Comment 27 WebKit Review Bot 2010-07-30 14:45:26 PDT
Attachment 63101 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3649049
Comment 28 WebKit Review Bot 2010-07-30 15:00:00 PDT
Attachment 63102 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3565734
Comment 29 chris fleizach 2010-07-30 15:03:10 PDT
Created attachment 63107 [details]
Patch
Comment 30 WebKit Review Bot 2010-07-30 15:04:46 PDT
Attachment 63102 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3577665
Comment 31 WebKit Review Bot 2010-07-30 15:37:03 PDT
Attachment 63107 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3640116
Comment 32 chris fleizach 2010-07-30 17:12:35 PDT
Created attachment 63129 [details]
Patch
Comment 33 David Kilzer (:ddkilzer) 2010-07-30 21:02:42 PDT
Comment on attachment 63129 [details]
Patch

WebCore/accessibility/Accessibility.cpp:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of

This third clause is no longer in the current license (see WebCore/LICENSE-APPLE).  Please update the license on all of the new files.

r=me
Comment 34 chris fleizach 2010-08-02 10:16:19 PDT
http://trac.webkit.org/changeset/64471
Comment 35 WebKit Review Bot 2010-08-02 10:41:49 PDT
http://trac.webkit.org/changeset/64471 might have broken Qt Linux Release
Comment 36 chris fleizach 2010-08-02 10:43:04 PDT
(In reply to comment #35)
> http://trac.webkit.org/changeset/64471 might have broken Qt Linux Release

didn't break it but changed some layout test results. will update
Comment 37 chris fleizach 2010-08-02 11:20:45 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > http://trac.webkit.org/changeset/64471 might have broken Qt Linux Release
> 
> didn't break it but changed some layout test results. will update

updated tests that would be changed by this change

http://trac.webkit.org/changeset/64478
Comment 38 chris fleizach 2010-08-02 11:45:14 PDT
(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > http://trac.webkit.org/changeset/64471 might have broken Qt Linux Release
> > 
> > didn't break it but changed some layout test results. will update
> 
> updated tests that would be changed by this change
> 
> http://trac.webkit.org/changeset/64478

QT is behaving wierdly. It's still failing

http://build.webkit.org/results/Qt%20Linux%20Release/r64480%20(16816)/results.html

but by all accounts it looks like it should have been fixed by

http://trac.webkit.org/changeset/64478. it would seem that that file was not updated?
Comment 39 Csaba Osztrogonác 2010-08-02 12:52:48 PDT
Qt has its own fast/dom/Window/window-properties-expected.txt expected file,
and was updated by http://trac.webkit.org/changeset/64486
Comment 40 Sam Weinig 2010-08-02 15:53:44 PDT
Hi Chris,

Without a spec or conversation with the rest of the WebKit community, I don't think we should be landing this in the tree, especially without any #define guards around it.  Please revert this until there has been the appropriate discussion.
Comment 41 David Kilzer (:ddkilzer) 2010-08-02 16:02:57 PDT
(In reply to comment #40)
> Hi Chris,
> 
> Without a spec or conversation with the rest of the WebKit community, I don't think we should be landing this in the tree, especially without any #define guards around it.  Please revert this until there has been the appropriate discussion.

Sorry, I should have not given this an r+ without more discussion about the API.

It would be nice to have a basic outline for the WebKit API review process posted on webkit.org for future reference, even if it's just a list of mailing lists where you should send proposals for each standards body.
Comment 42 chris fleizach 2010-08-02 17:30:44 PDT
Reverted r64486 for reason:

Rolling out https://bugs.webkit.org/show_bug.cgi?id=43005 until further discussion

Committed r64511: <http://trac.webkit.org/changeset/64511>
Comment 43 chris fleizach 2010-08-02 17:39:02 PDT
Reverted r64478 for reason:

Rolling out https://bugs.webkit.org/show_bug.cgi?id=43005 until further discussion

Committed r64512: <http://trac.webkit.org/changeset/64512>
Comment 44 WebKit Review Bot 2010-08-02 17:49:10 PDT
http://trac.webkit.org/changeset/64511 might have broken Qt Linux Release
Comment 45 chris fleizach 2010-08-02 17:50:02 PDT
(In reply to comment #44)
> http://trac.webkit.org/changeset/64511 might have broken Qt Linux Release

I'm rolling out all changes associated with this patch right now
Comment 46 chris fleizach 2010-08-02 17:51:26 PDT
Reverted r64476 for reason:

Rolling out https://bugs.webkit.org/show_bug.cgi?id=43005 until further discussion

Committed r64514: <http://trac.webkit.org/changeset/64514>
Comment 47 chris fleizach 2010-08-02 17:57:36 PDT
Reverted r64472 for reason:

Rolling out https://bugs.webkit.org/show_bug.cgi?id=43005 until further discussion

Committed r64515: <http://trac.webkit.org/changeset/64515>
Comment 48 chris fleizach 2010-08-02 18:05:44 PDT
Reverted r64471 for reason:

Rolling out https://bugs.webkit.org/show_bug.cgi?id=43005 until further discussion

Committed r64517: <http://trac.webkit.org/changeset/64517>
Comment 49 Sam Weinig 2010-08-02 18:25:10 PDT
Thanks Chris.
Comment 51 Adam Barth 2010-08-10 23:09:14 PDT
This bug is mysterious to me.  Does this patch need to be landed or do we want more review here?
Comment 52 David Kilzer (:ddkilzer) 2010-08-10 23:43:46 PDT
Comment on attachment 63129 [details]
Patch

Clearing r+ flag.
Comment 53 chris fleizach 2010-08-11 09:50:20 PDT
Comment on attachment 63129 [details]
Patch

going to hold off on this until we make a formal proposal
Comment 54 chris fleizach 2010-09-15 14:25:10 PDT
we have made the proposal public for a number of weeks now. no major criticisms

http://lists.w3.org/Archives/Public/wai-xtech/2010Aug/att-0079/UserInterfaceIndependence.html
Comment 55 chris fleizach 2010-09-15 15:35:11 PDT
Created attachment 67731 [details]
patch

This is the same patch that david kilzer reviewed before but also includes all the fixes i had made to get it to build on each platform.
Comment 56 Eric Seidel (no email) 2010-09-15 16:03:17 PDT
Attachment 67731 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4014022
Comment 57 chris fleizach 2010-09-15 16:06:00 PDT
Created attachment 67736 [details]
patch
Comment 58 Adam Barth 2011-01-12 15:50:24 PST
Comment on attachment 67736 [details]
patch

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

Seems reasonable.  Some nits below.

> WebCore/accessibility/AXObjectCache.h:47
> +#if PLATFORM(MAC)
> +#define SUPPORTS_JSACCESSIBILITY 1
> +#else
> +#define SUPPORTS_JSACCESSIBILITY 0
> +#endif

This isn't the way we usually enable features.  We usually use ENABLE macros?

> WebCore/accessibility/Accessibility.cpp:60
> +    Document* doc = m_frame->document();

Please use document as the variable name for the document.

> WebCore/accessibility/Accessibility.cpp:62
> +    if (!doc)
> +        return;

m_frame->document() can never be null.

> WebCore/accessibility/Accessibility.cpp:64
> +    AccessibilityObject* root = doc->axObjectCache()->getOrCreate(doc->renderer());

Is there any chance of this code running JavaScript?  If so, we need to hold a reference to the document.

> WebCore/accessibility/Accessibility.cpp:75
> +    if (!doc)
> +        return;

m_frame->document() can never be null.

> WebCore/accessibility/Accessibility.idl:33
> +

This line sounds extra.

> WebCore/accessibility/ScreenReader.h:47
> +private:

missing a newline above here.
Comment 59 James Craig 2013-09-30 17:46:38 PDT
Given that this section never made it into IndieUI, should we close this one as WONTFIX?
Comment 60 chris fleizach 2013-09-30 18:01:50 PDT
(In reply to comment #59)
> Given that this section never made it into IndieUI, should we close this one as WONTFIX?

Yes
Comment 61 James Craig 2013-09-30 18:50:24 PDT
Closing