Bug 121451

Summary:

AX: Expose ARIA Busy Notifications

Product:

WebKit

Reporter:

Samuel White <samuel_white>

Component:

Accessibility

Assignee:

Samuel White <samuel_white>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, ap, cfleizach, commit-queue, dmazzoni, jdiggs, mario, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.8

Bug Depends on:

121512

Bug Blocks:

Attachments:

Description	Flags
Patch.	none
Updated patch.	none
Updated patch.	none
Updated patch.	cfleizach: review+
Updated patch.	cfleizach: review+, commit-queue: commit-queue-
Updated patch.	none

Samuel White

Reported 2013-09-16 13:55:11 PDT

Webkit should emit a notification when an element's ARIA busy state has changed.

Attachments
Patch. (5.88 KB, patch) 2013-09-16 17:40 PDT, Samuel White	no flags	Details Formatted Diff Diff
Updated patch. (6.06 KB, patch) 2013-09-17 16:59 PDT, Samuel White	no flags	Details Formatted Diff Diff
Updated patch. (6.15 KB, patch) 2013-09-18 10:39 PDT, Samuel White	no flags	Details Formatted Diff Diff
Updated patch. (5.84 KB, patch) 2013-09-18 12:03 PDT, Samuel White	cfleizach: review+	Details Formatted Diff Diff
Updated patch. (5.89 KB, patch) 2013-09-20 16:30 PDT, Samuel White	cfleizach: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Updated patch. (5.89 KB, patch) 2013-09-30 21:02 PDT, Samuel White	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Samuel White

Comment 1 2013-09-16 13:59:19 PDT

<rdar://problem/14989411>

Samuel White

Comment 2 2013-09-16 17:40:38 PDT

Created attachment 211846 [details] Patch.

Samuel White

Comment 3 2013-09-16 17:45:40 PDT

I've named this notification AXElementBusyChanged rather than AXARIABusyChanged (which would align better with the AXARIABusy attribute) because of the desire to move away from the tight coupling with aria here. Evidence of this desire: https://bugs.webkit.org/show_bug.cgi?id=121442 After this patch is landed I intend to submit a patch to that bug to change AXARIABusy to AXElementBusy to bring everything inline.

Darin Adler

Comment 4 2013-09-17 08:58:01 PDT

Comment on attachment 211846 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211846&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:834 > + postNotification(element, AXObjectCache::AXElementBusyChanged, true); The boolean argument to postNotification really makes this code hard to review. We should change to use an enum instead since we are passing constants to it. If it was boolean expressions only then it would be fine to keep it a boolean.

Samuel White

Comment 5 2013-09-17 10:38:24 PDT

(In reply to comment #4) > (From update of attachment 211846 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211846&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:834 > > + postNotification(element, AXObjectCache::AXElementBusyChanged, true); > > The boolean argument to postNotification really makes this code hard to review. We should change to use an enum instead since we are passing constants to it. If it was boolean expressions only then it would be fine to keep it a boolean. Good point. I've opened https://bugs.webkit.org/show_bug.cgi?id=121504 to address this. Thanks.

WebKit Commit Bot

Comment 6 2013-09-17 11:09:45 PDT

Comment on attachment 211846 [details] Patch. Clearing flags on attachment: 211846 Committed r155976: <http://trac.webkit.org/changeset/155976>

WebKit Commit Bot

Comment 7 2013-09-17 11:09:48 PDT

All reviewed patches have been landed. Closing bug.

WebKit Commit Bot

Comment 8 2013-09-17 12:57:30 PDT

Re-opened since this is blocked by bug 121512

Alexey Proskuryakov

Comment 9 2013-09-17 13:01:05 PDT

Comment on attachment 211846 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211846&action=review This test is crashing with an assertion, will roll out. I'm not sure why EWS didn't catch this, but WK1 Release tester crashes with an assertion. It might be necessary to try a debug build locally. The reason for the assertion is that the test calls addNotificationListener, but not removeNotificationListener. The right long term fix might be to make it unnecessary, and to remove all listeners between tests automatically. For a term fix, a call to removeNotificationListener should be added where appropriate. > LayoutTests/platform/mac/accessibility/element-busy-changed.html:31 > + finishJSTest(); > + > + if (window.testRunner) > + testRunner.notifyDone(); > + } > + } > + > + if (window.testRunner) > + testRunner.waitUntilDone(); A correct way to write an async test is to first set window.jsTestIsAsync to true, and then call finishJSTest(). Explicit calls to testRunner.notifyDone and testRunner.waitUntilDone are not needed.

Alexey Proskuryakov

Comment 10 2013-09-17 13:18:34 PDT

Rolled out in <http://trac.webkit.org/changeset/155989>.

Samuel White

Comment 11 2013-09-17 16:59:26 PDT

Created attachment 211958 [details] Updated patch.

chris fleizach

Comment 12 2013-09-17 17:11:32 PDT

Comment on attachment 211958 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211958&action=review > LayoutTests/platform/mac/accessibility/element-busy-changed.html:8 > +<body aria-busy="false" id="body"> you still need to set window.jsTestIsAsync

Samuel White

Comment 13 2013-09-17 17:20:11 PDT

(In reply to comment #9) > (From update of attachment 211846 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211846&action=review > > This test is crashing with an assertion, will roll out. > > I'm not sure why EWS didn't catch this, but WK1 Release tester crashes with an assertion. It might be necessary to try a debug build locally. > > The reason for the assertion is that the test calls addNotificationListener, but not removeNotificationListener. The right long term fix might be to make it unnecessary, and to remove all listeners between tests automatically. For a term fix, a call to removeNotificationListener should be added where appropriate. This worked, thank you. > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:31 > > + finishJSTest(); > > + > > + if (window.testRunner) > > + testRunner.notifyDone(); > > + } > > + } > > + > > + if (window.testRunner) > > + testRunner.waitUntilDone(); > > A correct way to write an async test is to first set window.jsTestIsAsync to true, and then call finishJSTest(). Explicit calls to testRunner.notifyDone and testRunner.waitUntilDone are not needed. Once the load event fires the test dumps. Setting window.jsTestIsAsync to true isn't causing that process to hold like calling waitUntilDone does. So without that call the test is dumping before my notification callbacks happen. Thoughts? Thanks again for your help.

Samuel White

Comment 14 2013-09-17 17:31:54 PDT

(In reply to comment #12) > (From update of attachment 211958 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211958&action=review > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:8 > > +<body aria-busy="false" id="body"> > > you still need to set > window.jsTestIsAsync You beat my response. Please see comment #13. Thanks.

chris fleizach

Comment 15 2013-09-17 17:32:44 PDT

(In reply to comment #14) > (In reply to comment #12) > > (From update of attachment 211958 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=211958&action=review > > > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:8 > > > +<body aria-busy="false" id="body"> > > > > you still need to set > > window.jsTestIsAsync > > You beat my response. Please see comment #13. Thanks. You need to set both generally I think. At least all the other asynchronous tests I have done use both

Alexey Proskuryakov

Comment 16 2013-09-18 10:04:45 PDT

Got it, what's missing from this test is js-test-post.js at the end. So to recap, here is what an async test needs: - include js-test-pre.js at the top; - include js-test-post.js at the bottom (last line before </body>); - window.jsTestIsAsync = true at the beginning of your main test script; - finishJSTest() when done.

Samuel White

Comment 17 2013-09-18 10:39:33 PDT

Created attachment 212003 [details] Updated patch.

Samuel White

Comment 18 2013-09-18 10:41:12 PDT

(In reply to comment #16) > Got it, what's missing from this test is js-test-post.js at the end. > > So to recap, here is what an async test needs: > > - include js-test-pre.js at the top; > - include js-test-post.js at the bottom (last line before </body>); Added. > - window.jsTestIsAsync = true at the beginning of your main test script; Added. > - finishJSTest() when done. Thanks.

Alexey Proskuryakov

Comment 19 2013-09-18 10:49:30 PDT

Comment on attachment 212003 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212003&action=review > LayoutTests/platform/mac/accessibility/element-busy-changed.html:22 > + if (window.testRunner) > + testRunner.waitUntilDone(); Please remove this, it's not needed and thus confusing. > LayoutTests/platform/mac/accessibility/element-busy-changed.html:36 > + if (window.testRunner) > + testRunner.notifyDone(); Ditto. > LayoutTests/platform/mac/accessibility/element-busy-changed.html:47 > +<script src="../resources/js-test-post.js"></script> Hmm, why is the path different from the js-test-pre.js one?

Samuel White

Comment 20 2013-09-18 12:03:10 PDT

Created attachment 212009 [details] Updated patch.

Samuel White

Comment 21 2013-09-18 12:11:32 PDT

(In reply to comment #19) > (From update of attachment 212003 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212003&action=review > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:22 > > + if (window.testRunner) > > + testRunner.waitUntilDone(); > > Please remove this, it's not needed and thus confusing. > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:36 > > + if (window.testRunner) > > + testRunner.notifyDone(); > > Ditto. > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:47 > > +<script src="../resources/js-test-post.js"></script> > > Hmm, why is the path different from the js-test-pre.js one? This was the real issue (copy paste error). Once corrected 'window.jsTestIsAsync = true;' caused waitUntilDone() to be called as expected and finishJSTest() called notifyDone() as expected. I also removed 'wasPostTestScriptParsed = true;' as it is set implicitly in js-test-post.js as well. I'll file a bug to correct the other existing accessibility tests that I was using as a template.

Samuel White

Comment 22 2013-09-18 12:59:16 PDT

Bug to cleanup other offending accessibility tests: https://bugs.webkit.org/show_bug.cgi?id=121565

chris fleizach

Comment 23 2013-09-20 08:58:25 PDT

Comment on attachment 212009 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212009&action=review > LayoutTests/platform/mac/accessibility/element-busy-changed.html:27 > + shouldBe("elementBusyChangedCount", "2"); This seems like a gratuitous shouldBe() since the if statement already does this. I propose you just use a debug log to output that you received the notification (instead of a shouldBe) debug("Received: " + notification) that way if the test fails, we'll see in the diff right away that oh, we're missing one of those notifications

Samuel White

Comment 24 2013-09-20 16:30:42 PDT

Created attachment 212240 [details] Updated patch. Updated with feedback from Chris.

Samuel White

Comment 25 2013-09-20 16:32:56 PDT

(In reply to comment #23) > (From update of attachment 212009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212009&action=review > > > LayoutTests/platform/mac/accessibility/element-busy-changed.html:27 > > + shouldBe("elementBusyChangedCount", "2"); > > This seems like a gratuitous shouldBe() since the if statement already does this. > > I propose you just use a debug log to output that you received the notification (instead of a shouldBe) > > debug("Received: " + notification) > > that way if the test fails, we'll see in the diff right away that oh, we're missing one of those notifications I've replaced the shouldBe checks with some debug output that includes the number of notifications expected and the type received so tracking failures should be easier. Thanks.

WebKit Commit Bot

Comment 26 2013-09-27 15:52:01 PDT

WebKit Commit Bot

Comment 27 2013-09-30 11:06:11 PDT

Comment on attachment 212240 [details] Updated patch. Rejecting attachment 212240 [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-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: olumes/Data/EWS/WebKit/Source/WebCore/page/animation/AnimationController.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AnimationController.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o accessibility/AccessibilityAllInOne.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.appspot.com/results/2815103

Samuel White

Comment 28 2013-09-30 21:02:56 PDT

Created attachment 213050 [details] Updated patch. Rebase was needed to update the stale three lines of leading/trailing patch insertion text. Also removed 'true' argument to postNotification method as it is no longer correct. Other than that, no functional changes. Should be good to go.

WebKit Commit Bot

Comment 29 2013-09-30 22:56:50 PDT

Comment on attachment 213050 [details] Updated patch. Clearing flags on attachment: 213050 Committed r156698: <http://trac.webkit.org/changeset/156698>

WebKit Commit Bot

Comment 30 2013-09-30 22:56:55 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.