Trying to call setAccessibilityValue on a content editable or aria text control fails. That support was never built in <rdar://problem/31717256>
Created attachment 313218 [details] patch
Comment on attachment 313218 [details] patch Attachment 313218 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3950758 New failing tests: inspector/canvas/create-canvas-contexts.html
Created attachment 313221 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313218 [details] patch Attachment 313218 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3950768 New failing tests: accessibility/set-value-editable-types.html
Created attachment 313222 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 313218 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313218&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1754 > + else if (isARIATextControl()) A noted limitation of ARIA 1.x is that it is "one way." ATs can get values, but not set them. Should we really allow setting the value for non-content-editable ARIA text controls? And if your answer is "yes," then what about setting the value of ARIA range controls (which I believe James indicated couldn't, or shouldn't, be done for <insert reason I forgot here>)? Note that I'm not yet saying "Don't do this"; it's more of a "Really? Are you sure?"
(In reply to Joanmarie Diggs (irc: joanie) from comment #6) > what about setting the value of ARIA range controls (which I believe James > indicated couldn't, or shouldn't, be done for <insert reason I forgot here>)? > > Note that I'm not yet saying "Don't do this"; it's more of a "Really? Are > you sure?" Not sure if the reason James cited was the author updating their stored value, which would subsequently get sent as part of the form submission, but.... If it's not a content-editable control, and it's not a native host language control, and we set the value, what mechanism are we going to use to notify the author of the new value? And what guarantee do we have that the author will in turn accept that new value? (I'm afraid I'm starting to drift away from "Are you sure?" and towards "Don't do this.")
Comment on attachment 313218 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313218&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1755 > + element.setInnerHTML(string); I'm ok removing that part of this patch. I saw the FIXME in the code and realized it would be easy to change. I'm mostly concerned about the content editable part in this patch We can wait for AOM or future ARIA versions to worry about the content setting parts
Created attachment 313232 [details] patch Updated patch to remove aria text control changes
Comment on attachment 313232 [details] patch Attachment 313232 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3953542 New failing tests: accessibility/set-value-editable-types.html
Created attachment 313235 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313232 [details] patch Attachment 313232 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3953550 New failing tests: accessibility/set-value-editable-types.html
Created attachment 313238 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 313232 [details] patch Attachment 313232 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3953564 New failing tests: accessibility/set-value-editable-types.html
Created attachment 313239 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 313267 [details] patch
Comment on attachment 313267 [details] patch Attachment 313267 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3956028 New failing tests: accessibility/set-value-editable-types.html
Created attachment 313269 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 313270 [details] patch
Attachment 313270 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 313271 [details] patch
Joanmarie can you review again? thx
Comment on attachment 313271 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313271&action=review > LayoutTests/accessibility/set-value-editable-types.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use modern doctype: <!DOCTYPE html> > LayoutTests/accessibility/set-value-editable-types.html:5 > +<head> > +<script src="../resources/js-test-pre.js"></script> > +</head> No need for head. Just put it in body. > LayoutTests/accessibility/set-value-editable-types.html:20 > + if (window.accessibilityController) { You should add testFailed when window.accessibilityController is not defined with a description. > LayoutTests/accessibility/set-value-editable-types.html:25 > + debug("Role: " + axElement.role); > + debug("Value: " + axElement.stringValue); Please use shouldBe instead. > LayoutTests/accessibility/set-value-editable-types.html:28 > + var writable = axElement.isAttributeSettable("AXValue"); > + debug("Writable: " + writable); Ditto. > LayoutTests/accessibility/set-value-editable-types.html:36 > + // setting attributes can be done after a dispatch. > + window.setTimeout(function() { > + debug("Updated value: " + axElement.stringValue); > + completion(); > + }, 10); There is no callback for this? 10ms may not be enough when the load is heavy on the machine.
(In reply to Ryosuke Niwa from comment #23) > Comment on attachment 313271 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313271&action=review > > > LayoutTests/accessibility/set-value-editable-types.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Please use modern doctype: <!DOCTYPE html> > > > LayoutTests/accessibility/set-value-editable-types.html:5 > > +<head> > > +<script src="../resources/js-test-pre.js"></script> > > +</head> > > No need for head. Just put it in body. > > > LayoutTests/accessibility/set-value-editable-types.html:20 > > + if (window.accessibilityController) { > > You should add testFailed when window.accessibilityController is not defined > with a description. > > > LayoutTests/accessibility/set-value-editable-types.html:25 > > + debug("Role: " + axElement.role); > > + debug("Value: " + axElement.stringValue); > > Please use shouldBe instead. That doesn't scale as well to all the platforms. When we use debug, its easier to add expectations files for each platform rather than writing new tests for each platform > > > LayoutTests/accessibility/set-value-editable-types.html:28 > > + var writable = axElement.isAttributeSettable("AXValue"); > > + debug("Writable: " + writable); > > Ditto. > > > LayoutTests/accessibility/set-value-editable-types.html:36 > > + // setting attributes can be done after a dispatch. > > + window.setTimeout(function() { > > + debug("Updated value: " + axElement.stringValue); > > + completion(); > > + }, 10); > > There is no callback for this? 10ms may not be enough when the load is heavy > on the machine. There's no callback for calling the accessibilitySetValue. It just happens on the next iteration of the run loop, so I think if we can delay our JS method to another run loop iteration, this will happen in order
Created attachment 313417 [details] patch Addressed relevant comments
Comment on attachment 313417 [details] patch Attachment 313417 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3967368 New failing tests: imported/w3c/IndexedDB-private-browsing/idbobjectstore_clear2.html
Created attachment 313431 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 313417 [details] patch Nice!
Comment on attachment 313417 [details] patch Clearing flags on attachment: 313417 Committed r218633: <http://trac.webkit.org/changeset/218633>
All reviewed patches have been landed. Closing bug.
After this revision was landed, the test accessibility/set-value-editable-types.html started to consistently fail on Windows Release and Sierra WK2 Debug. https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fset-value-editable-types.html https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r218643%20(1676)/results.html Sierra Diff: --- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/accessibility/set-value-editable-types-expected.txt +++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/accessibility/set-value-editable-types-actual.txt @@ -8,7 +8,7 @@ Role: AXRole: AXTextArea Value: AXValue: current1 Writable: true -Updated value: AXValue: new value +Updated value: AXValue: current1 PASS successfullyParsed is true TEST COMPLETE Windows Diff: --- /home/buildbot/slave/win-release-tests/build/layout-test-results/accessibility/set-value-editable-types-expected.txt +++ /home/buildbot/slave/win-release-tests/build/layout-test-results/accessibility/set-value-editable-types-actual.txt @@ -5,10 +5,10 @@ Contenteditable -Role: AXRole: AXTextArea +Role: AXRole: AXStaticText Value: AXValue: current1 -Writable: true -Updated value: AXValue: new value +Writable: false +Updated value: AXValue: current1 PASS successfullyParsed is true TEST COMPLETE
Reverted r218633 for reason: The test is failing frequently on Sierra Debug and Windows Committed r218702: <http://trac.webkit.org/changeset/218702>
(In reply to Matt Lewis from comment #32) > Reverted r218633 for reason: > > The test is failing frequently on Sierra Debug and Windows > > Committed r218702: <http://trac.webkit.org/changeset/218702> Completely unhelpful. Test on windows should just be skipped
(In reply to chris fleizach from comment #33) > (In reply to Matt Lewis from comment #32) > > Reverted r218633 for reason: > > > > The test is failing frequently on Sierra Debug and Windows > > > > Committed r218702: <http://trac.webkit.org/changeset/218702> > > Completely unhelpful. Test on windows should just be skipped We can skip the test on Windows, but it was still a flaky failure on Sierra Debug WK2.
(In reply to Ryan Haddad from comment #34) > (In reply to chris fleizach from comment #33) > > (In reply to Matt Lewis from comment #32) > > > Reverted r218633 for reason: > > > > > > The test is failing frequently on Sierra Debug and Windows > > > > > > Committed r218702: <http://trac.webkit.org/changeset/218702> > > > > Completely unhelpful. Test on windows should just be skipped > > We can skip the test on Windows, but it was still a flaky failure on Sierra > Debug WK2. I'm working on a fix for both and will post a new patch for review thanks
Created attachment 313652 [details] patch New patch - move the test to Mac only - rely on the value change callback before we check the updated value
Anyone available to review this again?
Comment on attachment 313652 [details] patch Thanks Ryosuke!
Comment on attachment 313652 [details] patch Rejecting attachment 313652 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 313652, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 22f M Source :040000 040000 5bf598c00c17d61e47dc225a4161bef0a48128e7 51df1e4d97692ef22e6c26445eba3e4fc4ba1f8f M Tools Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/3987435
Comment on attachment 313652 [details] patch Clearing flags on attachment: 313652 Committed r218783: <http://trac.webkit.org/changeset/218783>
It looks like this patch caused test: accessibility/mac/setting-attributes-is-asynchronous.html to crash every time on macOS WK2 Debug. It's starting to affect testing. https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218804%20(1939)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Fsetting-attributes-is-asynchronous.html stderr: 1 0x11a79d930 WTFCrash 2 0x121d02de7 WTR::AccessibilityController::removeNotificationListener() 3 0x121d3c285 WTR::JSAccessibilityController::removeNotificationListener(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) 4 0x11a0fcd69 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) 5 0x11a2d75b3 JSC::LLInt::handleHostCall(JSC::ExecState*, JSC::Instruction*, JSC::JSValue, JSC::CodeSpecializationKind) 6 0x11a2d87c0 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) 7 0x11a2d8396 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind) 8 0x11a2d47a9 llint_slow_path_call 9 0x11a2e36b0 llint_entry 10 0x11a2dbd6e vmEntryToJavaScript 11 0x11a09c341 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 12 0x11a0497d5 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 13 0x1197d2ebe JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 14 0x1197d30fa JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 15 0x11a1ce9ed JSObjectCallAsFunction 16 0x121d03df0 -[AccessibilityNotificationHandler _notificationReceived:] 17 0x7fff88554b0c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ 18 0x7fff88554a9f ___CFXRegistrationPost_block_invoke 19 0x7fff88554a17 _CFXRegistrationPost 20 0x7fff88554782 ___CFXNotificationPost_block_invoke 21 0x7fff88511592 -[_CFXNotificationRegistrar find:object:observer:enumerator:] 22 0x7fff885107e5 _CFXNotificationPost 23 0x7fff91b2323a -[NSNotificationCenter postNotificationName:object:userInfo:] 24 0x10fdeb8ab -[WebAccessibilityObjectWrapperBase accessibilityPostedNotification:userInfo:] 25 0x10d36c525 WebCore::AXPostNotificationWithUserInfo(WebAccessibilityObjectWrapper*, NSString*, objc_object*) 26 0x10d36c415 WebCore::AXObjectCache::postPlatformNotification(WebCore::AccessibilityObject*, WebCore::AXObjectCache::AXNotification) 27 0x10d33ba4d WebCore::AXObjectCache::notificationPostTimerFired() 28 0x10d354bc7 WTF::Function<void ()>::CallableWrapper<std::__1::__bind<void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*> >::call() 29 0x10d1de093 WTF::Function<void ()>::operator()() const 30 0x10d1ddefc WebCore::Timer::fired() 31 0x10fcb21c2 WebCore::ThreadTimers::sharedTimerFiredInternal() LEAK: 1 WebPageProxy
Reverted r218783 for reason: Causing accessibility/mac/setting-attributes-is-asynchronous.html to crash consistently on mac-wk2 Debug Committed r218817: <http://trac.webkit.org/changeset/218817>
I'm going to look into this failure a bit today. Looking over the content of this patch briefly, I suspect something wrong with infrastructure, but in the mean time, rolling out this change since it's pretty consistently causing WebKit 2 to crash on Mac.
(In reply to Jonathan Bedard from comment #44) > I'm going to look into this failure a bit today. Looking over the content > of this patch briefly, I suspect something wrong with infrastructure, but in > the mean time, rolling out this change since it's pretty consistently > causing WebKit 2 to crash on Mac. can we not do that since this is just a test issue.
(In reply to chris fleizach from comment #45) > (In reply to Jonathan Bedard from comment #44) > > I'm going to look into this failure a bit today. Looking over the content > > of this patch briefly, I suspect something wrong with infrastructure, but in > > the mean time, rolling out this change since it's pretty consistently > > causing WebKit 2 to crash on Mac. > > can we not do that since this is just a test issue. Our policy is to rollout any patch that recesses our tests.
(In reply to chris fleizach from comment #45) > (In reply to Jonathan Bedard from comment #44) > > I'm going to look into this failure a bit today. Looking over the content > > of this patch briefly, I suspect something wrong with infrastructure, but in > > the mean time, rolling out this change since it's pretty consistently > > causing WebKit 2 to crash on Mac. > > can we not do that since this is just a test issue. Just managed to get to the bottom of this. You're calling finishJSTest() too early, it should be called after accessibilityController.removeNotificationListener(). Calling removeNotificationListener() will actually cause the next test to crash. If finishJSTest() is called after accessibilityController.removeNotificationListener(), I no longer see the test failure locally. Ryosuke, do you have any other additional comments, or do you think this patch is ready to go with that change?
(In reply to Jonathan Bedard from comment #47) > (In reply to chris fleizach from comment #45) > > (In reply to Jonathan Bedard from comment #44) > > > I'm going to look into this failure a bit today. Looking over the content > > > of this patch briefly, I suspect something wrong with infrastructure, but in > > > the mean time, rolling out this change since it's pretty consistently > > > causing WebKit 2 to crash on Mac. > > > > can we not do that since this is just a test issue. > > Just managed to get to the bottom of this. > > You're calling finishJSTest() too early, it should be called after > accessibilityController.removeNotificationListener(). Calling > removeNotificationListener() will actually cause the next test to crash. If > finishJSTest() is called after > accessibilityController.removeNotificationListener(), I no longer see the > test failure locally. > > Ryosuke, do you have any other additional comments, or do you think this > patch is ready to go with that change? That makes sense. thanks for taking a second look.
My r=me stands. Please land the patch with the test fix.
(In reply to Jonathan Bedard from comment #47) > (In reply to chris fleizach from comment #45) > > (In reply to Jonathan Bedard from comment #44) > > > I'm going to look into this failure a bit today. Looking over the content > > > of this patch briefly, I suspect something wrong with infrastructure, but in > > > the mean time, rolling out this change since it's pretty consistently > > > causing WebKit 2 to crash on Mac. > > > > can we not do that since this is just a test issue. > > Just managed to get to the bottom of this. > > You're calling finishJSTest() too early, it should be called after > accessibilityController.removeNotificationListener(). Calling > removeNotificationListener() will actually cause the next test to crash. If > finishJSTest() is called after > accessibilityController.removeNotificationListener(), I no longer see the > test failure locally. > > Ryosuke, do you have any other additional comments, or do you think this > patch is ready to go with that change? Jonathan are you going to land this patch again with your change?
(In reply to chris fleizach from comment #50) > ... > > Jonathan are you going to land this patch again with your change? I was not going to, although I can. Since it's your change, Chris, I think you should be the one to land it.
Created attachment 314121 [details] patch for landing
Comment on attachment 314121 [details] patch for landing Clearing flags on attachment: 314121 Committed r218986: <http://trac.webkit.org/changeset/218986>