Bug 98357 – [GTK] accessibility/aria-readonly.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

accessibility/aria-readonly.html is failing on all GTK platforms.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Faria-readonly.html

[Joanmarie Diggs]

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

[Joanmarie Diggs]

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

[chris fleizach]

(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

[Joanmarie Diggs]

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

[James Craig]

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.

[Joanmarie Diggs]

Created attachment 179254 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 179254 [details]
Patch

Chris, please review (EWS is all green).

[chris fleizach]

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 ?

[Joanmarie Diggs]

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

[chris fleizach]

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!

[Joanmarie Diggs]

Created attachment 179566 [details]
Patch

[Joanmarie Diggs]

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

[Build Bot]

Comment on attachment 179566 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 179566 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 179566 [details]
Patch

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

[Joanmarie Diggs]

Created attachment 179583 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179583 [details]
Patch

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

[Joanmarie Diggs]

Created attachment 179584 [details]
Patch

[Joanmarie Diggs]

Created attachment 179586 [details]
Patch

[Joanmarie Diggs]

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!)

[WebKit Review Bot]

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

[Mario Sanchez Prada]

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?

[Joanmarie Diggs]

Created attachment 278413 [details]
Patch

[Joanmarie Diggs]

Created attachment 278437 [details]
Patch

[Joanmarie Diggs]

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!

[chris fleizach]

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

[Joanmarie Diggs]

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!!

[Joanmarie Diggs]

Created attachment 278581 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 278581 [details]
Patch

Clearing flags on attachment: 278581

Committed r200677: <http://trac.webkit.org/changeset/200677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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.

[Joanmarie Diggs]

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

[chris fleizach]

(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