Summary: | AX: Support methods for web apps to interact with the native accessibility APIs | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, bdakin, ctguil, darin, ddkilzer, dglazkov, eric, gustavo, 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
chris fleizach
2010-07-26 14:39:26 PDT
this initial patch will fail on a lot of systems i imagine. will gather build output to fix Created attachment 62612 [details]
Patch
(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 on attachment 62612 [details]
Patch
r- to post another patch for the bots.
(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 Created attachment 63068 [details]
Patch
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.
Attachment 63068 [details] did not build on qt: Build output: http://queues.webkit.org/results/3610443 Created attachment 63074 [details]
Patch
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.
Attachment 63074 [details] did not build on qt: Build output: http://queues.webkit.org/results/3574705 Created attachment 63082 [details]
Patch
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.
Attachment 63068 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3590631 Attachment 63082 [details] did not build on qt: Build output: http://queues.webkit.org/results/3633195 Created attachment 63086 [details]
Patch
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.
Attachment 63086 [details] did not build on qt: Build output: http://queues.webkit.org/results/3580603 Attachment 63086 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3590635 Attachment 63086 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3573632 Created attachment 63101 [details]
Patch
Attachment 63101 [details] did not build on qt: Build output: http://queues.webkit.org/results/3634161 Attachment 63101 [details] did not build on mac: Build output: http://queues.webkit.org/results/3561639 Created attachment 63102 [details]
Patch
Attachment 63102 [details] did not build on qt: Build output: http://queues.webkit.org/results/3611423 Attachment 63101 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3572654 Attachment 63101 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3649049 Attachment 63102 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3565734 Created attachment 63107 [details]
Patch
Attachment 63102 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3577665 Attachment 63107 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3640116 Created attachment 63129 [details]
Patch
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
http://trac.webkit.org/changeset/64471 might have broken Qt Linux Release (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 (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 (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? Qt has its own fast/dom/Window/window-properties-expected.txt expected file, and was updated by http://trac.webkit.org/changeset/64486 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. (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. 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> 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> http://trac.webkit.org/changeset/64511 might have broken Qt Linux Release (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 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> 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> 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> Thanks Chris. http://trac.webkit.org/changeset/64517 might have broken Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/64517 http://trac.webkit.org/changeset/64518 http://trac.webkit.org/changeset/64519 http://trac.webkit.org/changeset/64520 http://trac.webkit.org/changeset/64521 http://trac.webkit.org/changeset/64522 http://trac.webkit.org/changeset/64523 http://trac.webkit.org/changeset/64524 http://trac.webkit.org/changeset/64525 http://trac.webkit.org/changeset/64526 http://trac.webkit.org/changeset/64527 This bug is mysterious to me. Does this patch need to be landed or do we want more review here? Comment on attachment 63129 [details]
Patch
Clearing r+ flag.
Comment on attachment 63129 [details]
Patch
going to hold off on this until we make a formal proposal
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 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.
Attachment 67731 [details] did not build on mac: Build output: http://queues.webkit.org/results/4014022 Created attachment 67736 [details]
patch
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. Given that this section never made it into IndieUI, should we close this one as WONTFIX? (In reply to comment #59) > Given that this section never made it into IndieUI, should we close this one as WONTFIX? Yes Closing |