Bug 98357

Created attachment 178569 [details] test case for discussion While working on this bug, I came across the following issue: aria-readonly is independent of the element's readonly state. According to the W3C [1], aria-readonly: Indicates that the element is not editable, but is otherwise operable. See related aria-disabled. This means the user can read but not set the value of the widget. Readonly elements are relevant to the user, and application authors SHOULD NOT restrict navigation to the element or its focusable descendants. Other actions such as copying the value of the element are also supported. This is in contrast to disabled elements, to which applications might not allow user navigation to descendants. Because the readonly state in reality is completely disconnected from aria-readonly, an assistive technology is in danger of informing the user that a widget which is editable is not editable. This strikes me as highly undesirable. I bring this up for the following reason: The current failing test is failing because we need to implement AccessibilityUIElement::isAttributeSettable(). I implemented it as follows: bool AccessibilityUIElement::isAttributeSettable(JSStringRef attribute) { // In Atk, things are generally settable if they are not read-only. At the // moment, we identify read only via ATK_STATE_EDITABLE. In the future, // once it is implemented, ATK_STATE_READ_ONLY should also be checked: // https://bugzilla.gnome.org/show_bug.cgi?id=665598 return checkElementState(m_element, ATK_STATE_EDITABLE); } That gets the test assertions in aria-readonly.html passing with one exception: PASS ariaTextBoxIsWritable is true PASS ariaReadOnlyAriaTextBoxIsWritable is false -PASS ariaReadOnlyTextFieldIsWritable is false +FAIL ariaReadOnlyTextFieldIsWritable should be false. Was true. PASS ariaNonReadOnlyTextFieldIsWritable is true PASS htmlReadOnlyTextFieldIsWritable is false PASS textFieldIsWritable is true In the case of the failure, ariaReadOnlyTextFieldIsWritable actually IS writable. A user can type in it. Thus it should not be false IMHO. Making this test pass as written by modifying the ATK port would have a side effect of making the Orca screen reader telling the user a non-readonly widget is readonly. I consider that unacceptable. Therefore, what should we do? Should the element respect the ARIA value and make the widget truly readonly? Should the Accessibility code ignore ARIA attributes that do not jive with reality? Should we move the test to the Mac port (where it fully passes at the moment) and provide a new test which lacks the problematic item? Or something else? Thoughts? [1] http://www.w3.org/TR/wai-aria/states_and_properties#aria-readonly

Chris could you please weigh in on my comment 1? Thanks!

(In reply to comment #1) > Created an attachment (id=178569) [details] > test case for discussion > > While working on this bug, I came across the following issue: aria-readonly is independent of the element's readonly state. > > According to the W3C [1], aria-readonly: > > Indicates that the element is not editable, but is otherwise > operable. See related aria-disabled. > > This means the user can read but not set the value of the widget. > Readonly elements are relevant to the user, and application authors > SHOULD NOT restrict navigation to the element or its focusable > descendants. Other actions such as copying the value of the element > are also supported. This is in contrast to disabled elements, to > which applications might not allow user navigation to descendants. > > Because the readonly state in reality is completely disconnected from aria-readonly, an assistive technology is in danger of informing the user that a widget which is editable is not editable. This strikes me as highly undesirable. > > I bring this up for the following reason: The current failing test is failing because we need to implement AccessibilityUIElement::isAttributeSettable(). I implemented it as follows: > > bool AccessibilityUIElement::isAttributeSettable(JSStringRef attribute) > { > // In Atk, things are generally settable if they are not read-only. At the > // moment, we identify read only via ATK_STATE_EDITABLE. In the future, > // once it is implemented, ATK_STATE_READ_ONLY should also be checked: > // https://bugzilla.gnome.org/show_bug.cgi?id=665598 > > return checkElementState(m_element, ATK_STATE_EDITABLE); > } Does this get set from calling AccessibilityObject::isAttributeSettable()? Ideally, you'd like your implementation to match that. I'm not sure editable and read-only are always the same thing, and I believe there's also other non settable cases that don't pertain to aria-read only or text fields... > > That gets the test assertions in aria-readonly.html passing with one exception: > > PASS ariaTextBoxIsWritable is true > PASS ariaReadOnlyAriaTextBoxIsWritable is false > -PASS ariaReadOnlyTextFieldIsWritable is false > +FAIL ariaReadOnlyTextFieldIsWritable should be false. Was true. > PASS ariaNonReadOnlyTextFieldIsWritable is true > PASS htmlReadOnlyTextFieldIsWritable is false > PASS textFieldIsWritable is true > > In the case of the failure, ariaReadOnlyTextFieldIsWritable actually IS writable. A user can type in it. Thus it should not be false IMHO. > > Making this test pass as written by modifying the ATK port would have a side effect of making the Orca screen reader telling the user a non-readonly widget is readonly. I consider that unacceptable. Therefore, what should we do? > > Should the element respect the ARIA value and make the widget truly readonly? Should the Accessibility code ignore ARIA attributes that do not jive with reality? Should we move the test to the Mac port (where it fully passes at the moment) and provide a new test which lacks the problematic item? Or something else? > I am not sure what is the right behavior. I'll get James craig from ARIA W3C group to check > Thoughts? > > [1] http://www.w3.org/TR/wai-aria/states_and_properties#aria-readonly

(In reply to comment #3) > Does this get set from calling AccessibilityObject::isAttributeSettable()? ATK_STATE_EDITABLE is set here: From http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp#L632: // FIXME: isReadOnly does not seem to do the right thing for // controls, so check explicitly for them. In addition, because // isReadOnly is false for listBoxOptions, we need to add one // more check so that we do not present them as being "editable". if ((!coreObject->isReadOnly() || (coreObject->isControl() && coreObject->canSetValueAttribute())) && !isListBoxOption) atk_state_set_add_state(stateSet, ATK_STATE_EDITABLE); > Ideally, you'd like your implementation to match that. I'm not sure editable and read-only are always the same thing, Agreed. We have a bug to add a new state in ATK specific to readonly: https://bugzilla.gnome.org/show_bug.cgi?id=665598 > I am not sure what is the right behavior. I'll get James craig from ARIA W3C group to check Awesome. Thank you!! I also pinged Joseph Scheuhammer (GNOME Accessibility Team guy who I believe also participates in that group). I will feel better having an official verdict. (Though I am still highly uncomfortable having a screen reader report something as read-only/non-editable when in reality it is editable.)

If @aria-readonly is set to false, the accessibility API should report that the item is readonly, even in the case where the item is actually writeable. ARIA allows the author to set specific overrides, even if they are wrong or conflict with native behavior. In this case, the author could script the "readonly" behavior on change or on keypress, so the rendering engine needs to report what the author explicitly added, whether or not the rendering engine thinks it conflicts with native functionality. Minor nit with the test case: id="htmlReadOnlyTextArea" appears twice in the document, so it's not validating or showing up in the inspector correctly.

Created attachment 179254 [details] Patch

Comment on attachment 179254 [details] Patch Chris, please review (EWS is all green).

Comment on attachment 179254 [details] Patch There's probably already code in AXrender object to check aria read only. Can that be factored to use the Node object implementation ?

(In reply to comment #8) > (From update of attachment 179254 [details]) > There's probably already code in AXrender object to check aria read only. Can that be factored to use the Node object implementation ? It already uses it, with the exception of the WebArea: ================================================== bool AccessibilityRenderObject::isReadOnly() const { ASSERT(m_renderer); if (isWebArea()) { Document* document = m_renderer->document(); if (!document) return true; HTMLElement* body = document->body(); if (body && body->rendererIsEditable()) return false; return !document->rendererIsEditable(); } return AccessibilityNodeObject::isReadOnly(); } ================================================== Are you suggesting we move the handling of the WebArea to the node object code?

Comment on attachment 179254 [details] Patch ok, looked at the code. i see that in AccessibilityRenderObject::canSetValueAttribute we're already checking for aria_readonly == true, which looks like it should be the entry point for this attribute. I also see that we want to check aria_readonly before everything else, so it's a little tricky to only put it in AXNodeObject, and obviously we shouldn't duplicate that check. So, I think maybe we should remove the isReadOnly() method and just use canSetAttributeValue() and move all that logic (and the method) into AXNodeObject. I don't see anything that absolutely requires being inside of AXRenderObject. That way we can implement the aria_readonly stuff first. we also need a new test (or modify aria-readonly.html) to test the aria-readonly=false case which will (after your patch) override native semantics thanks!

Created attachment 179566 [details] Patch

(In reply to comment #10) > So, I think maybe we should remove the isReadOnly() method and just use canSetAttributeValue() and move all that logic (and the method) into AXNodeObject. Done. Thanks for the tip! > I don't see anything that absolutely requires being inside of AXRenderObject. Here's to hoping the EWS doesn't either. ;) > we also need a new test (or modify aria-readonly.html) to test the aria-readonly=false case which will (after your patch) override native semantics Actually, that test already has two instances of aria-readonly=false. > thanks! And to you.

Comment on attachment 179566 [details] Patch Attachment 179566 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15321843

Comment on attachment 179566 [details] Patch Attachment 179566 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15320811

Comment on attachment 179566 [details] Patch Attachment 179566 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15311904

Created attachment 179583 [details] Patch

Comment on attachment 179583 [details] Patch Attachment 179583 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15323991

Created attachment 179584 [details] Patch

Created attachment 179586 [details] Patch

Dominic: I keep managing to break your port with my patch. :-/ And I don't (yet) have a chromium build environment for webkit. Could you please take a look to see where I'm going wrong? (Thanks in advance!)

Comment on attachment 179586 [details] Patch Attachment 179586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15363108 New failing tests: platform/chromium/accessibility/readonly.html

Comment on attachment 179586 [details] Patch Clearing r? so it does not appear in webkit.org/pending-review. Joanie, do you want to give it another try?

Created attachment 278413 [details] Patch

Created attachment 278437 [details] Patch

Chris: Even though the summary is prefixed with "[GTK]", the bulk of the changes are in WebCore Accessibility. So when you have some spare cycles, a review would be greatly appreciated. Thanks!

Comment on attachment 278437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278437&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:858 > + && (coreObject->supportsARIAReadOnly() || coreObject->hasAttribute(HTMLNames::aria_readonlyAttr)))) Why do you need two checks here supportsARIAReadOnly/hasAttribute It would be nice to keep HTML specific stuff out of platform wrappers, so that you just interface with the accessibility objects

Comment on attachment 278437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278437&action=review >> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:858 >> + && (coreObject->supportsARIAReadOnly() || coreObject->hasAttribute(HTMLNames::aria_readonlyAttr)))) > > Why do you need two checks here supportsARIAReadOnly/hasAttribute > It would be nice to keep HTML specific stuff out of platform wrappers, so that you just interface with the accessibility objects Aha. That is a leftover. The reason why it was there is because the aria-readonly.html test has a number of elements whose explicit or implicit role value is not one which supports aria-readonly according to the spec. I can remove that additional check because what I wound up doing is always exposing author-provided values as an object attribute. That way ATs (and AccessibilityUIElement) can still access the value independent from the states. I'll do a new patch because in addition to the above, I just discovered that ensuring your platform's toggleable widgets were not value-settable made my platform's toggleable widgets have STATE_READ_ONLY. Oops. Thanks for the review!!

Created attachment 278581 [details] Patch

Comment on attachment 278581 [details] Patch Clearing flags on attachment: 278581 Committed r200677: <http://trac.webkit.org/changeset/200677>

All reviewed patches have been landed. Closing bug.

This caused inspector/dom/getAccessibilityPropertiesForNode.html to fail on Mac: <https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/5929> It didn't look like a regression, so I rebaselined it in <https://trac.webkit.org/r200698> If this was incorrect, please let me know.

(In reply to comment #31) > This caused inspector/dom/getAccessibilityPropertiesForNode.html to fail on > Mac: > <https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/5929> > > It didn't look like a regression, so I rebaselined it in > <https://trac.webkit.org/r200698> > > If this was incorrect, please let me know. Apologies for missing that! Personally, I don't think it is a regression -- assuming that the purpose of that test is to output the ARIA properties that are present on the elements. That element has aria-disabled (which is being output), but lacks aria-readonly. That said, if Chris thinks it is a regression and he comments here to that effect, I'll do a new patch to address that.

(In reply to comment #32) > (In reply to comment #31) > > This caused inspector/dom/getAccessibilityPropertiesForNode.html to fail on > > Mac: > > <https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/5929> > > > > It didn't look like a regression, so I rebaselined it in > > <https://trac.webkit.org/r200698> > > > > If this was incorrect, please let me know. > > Apologies for missing that! > > Personally, I don't think it is a regression -- assuming that the purpose of > that test is to output the ARIA properties that are present on the elements. > That element has aria-disabled (which is being output), but lacks > aria-readonly. > > That said, if Chris thinks it is a regression and he comments here to that > effect, I'll do a new patch to address that. If anything it seems like an existing bug