Bug 173520 – AX: Cannot call setValue() on contenteditable or ARIA text controls

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Trying to call setAccessibilityValue on a content editable or aria text control fails. That support was never built in

<rdar://problem/31717256>

[chris fleizach]

Created attachment 313218 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joanmarie Diggs]

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?"

[Joanmarie Diggs]

(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.")

[chris fleizach]

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

[chris fleizach]

Created attachment 313232 [details]
patch

Updated patch to remove aria text control changes

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

Created attachment 313267 [details]
patch

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

Created attachment 313270 [details]
patch

[Build Bot]

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.

[chris fleizach]

Created attachment 313271 [details]
patch

[chris fleizach]

Joanmarie can you review again? thx

[Ryosuke Niwa]

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.

[chris fleizach]

(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

[chris fleizach]

Created attachment 313417 [details]
patch

Addressed relevant comments

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

Comment on attachment 313417 [details]
patch

Nice!

[WebKit Commit Bot]

Comment on attachment 313417 [details]
patch

Clearing flags on attachment: 313417

Committed r218633: <http://trac.webkit.org/changeset/218633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[Matt Lewis]

Reverted r218633 for reason:

The test is failing frequently on Sierra Debug and Windows

Committed r218702: <http://trac.webkit.org/changeset/218702>

[chris fleizach]

(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

[Ryan Haddad]

(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.

[chris fleizach]

(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

[chris fleizach]

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

[chris fleizach]

Anyone available to review this again?

[chris fleizach]

Comment on attachment 313652 [details]
patch

Thanks Ryosuke!

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 313652 [details]
patch

Clearing flags on attachment: 313652

Committed r218783: <http://trac.webkit.org/changeset/218783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[Jonathan Bedard]

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>

[Jonathan Bedard]

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.

[chris fleizach]

(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.

[Ryosuke Niwa]

(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.

[Jonathan Bedard]

(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?

[chris fleizach]

(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.

[Ryosuke Niwa]

My r=me stands. Please land the patch with the test fix.

[chris fleizach]

(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?

[Jonathan Bedard]

(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.

[chris fleizach]

Created attachment 314121 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 314121 [details]
patch for landing

Clearing flags on attachment: 314121

Committed r218986: <http://trac.webkit.org/changeset/218986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.