| 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: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Mac | ||||||||||||||||
| OS: | OS X 10.8 | ||||||||||||||||
| Bug Depends on: | 121512 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Samuel White
2013-09-16 13:55:11 PDT
Created attachment 211846 [details]
Patch.
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. 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. (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. Comment on attachment 211846 [details] Patch. Clearing flags on attachment: 211846 Committed r155976: <http://trac.webkit.org/changeset/155976> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 121512 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. Rolled out in <http://trac.webkit.org/changeset/155989>. Created attachment 211958 [details]
Updated patch.
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 (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. (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. (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 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. Created attachment 212003 [details]
Updated patch.
(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. 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? Created attachment 212009 [details]
Updated patch.
(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. Bug to cleanup other offending accessibility tests: https://bugs.webkit.org/show_bug.cgi?id=121565 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 Created attachment 212240 [details]
Updated patch.
Updated with feedback from Chris.
(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. 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-03', 'validate-changelog', '--check-oops', '--non-interactive', 212240, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/2649301 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 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.
Comment on attachment 213050 [details] Updated patch. Clearing flags on attachment: 213050 Committed r156698: <http://trac.webkit.org/changeset/156698> All reviewed patches have been landed. Closing bug. |