[ATK] Expose aria-invalid as a text attribute (not object attribute)

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

According to WAI-ARIA spec [1], this should not be exposed as an attribute of AtkObject, but as a text attribute, which is part of the AtkText interface.

So, we need to change it in the wrapper and update DRT/WKTR for the aria-invalid.html test to keep passing.

[1] http://www.w3.org/TR/wai-aria-implementation/#mapping_state-property

[Radar WebKit Bug Importer]

<rdar://problem/15098367>

[Mario Sanchez Prada]

Created attachment 212816 [details]
Patch proposal

Here comes the patch for this issue. Next step will be to expose aria-invalid as well as an state, but that will belong to a new bug, which I will most likely work on during the following week.

[chris fleizach]

Comment on attachment 212816 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=212816&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:308
> +        String invalidValue = invalidStatus == "spelling" || invalidStatus == "grammar" ? invalidStatus : "true";

i think invalid could be anything, so i think you want to just use the actual value instead of casting other types to true

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:69
> +static String atkAttributeValueToCoreAttributeValue(AtkAttributeType type, String& id, String& value)

these params look like they should be const

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:75
> +        if (id == "sort" && !value.isEmpty()) {

Maybe you should have at the top of some file a list of these hard coded strings like

const String atkSortAttributeName = "sort";

so that you're not referencing literals

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:86
> +    if (type == AtkAttributeTypeText) {

this should be an else if, since the first if does not always return

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:93
> +    // Return the passed value unchanged if not filtered.

this comment seems unnecessary unless you say why you are passing it unfiltered

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:90
> +    if (type == AtkAttributeTypeText) {

ditto about else if

[Mario Sanchez Prada]

(In reply to comment #3)
> (From update of attachment 212816 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212816&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:308
> > +        String invalidValue = invalidStatus == "spelling" || invalidStatus == "grammar" ? invalidStatus : "true";
> 
> i think invalid could be anything, so i think you want to just use the actual 
> value instead of casting other types to true

Yeah, I thought of that, but the WAI-ARIA spec explicitly says in [1] to do it the other way:

"For future expansion, the aria-invalid attribute is an enumerated type. Any value not recognized in the list of allowed values MUST be treated by user agents as if the value true had been provided. If the attribute is not present, or its value is false, or its value is an empty string, the default value of false applies."

So my understanding is that we only expose 'grammar' or 'spelling' if those values are set, and 'true' otherwise.

[1] http://www.w3.org/TR/2011/CR-wai-aria-20110118/states_and_properties#aria-invalid

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:69
> > +static String atkAttributeValueToCoreAttributeValue(AtkAttributeType type, String& id, String& value)
> 
> these params look like they should be const
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:75
> > +        if (id == "sort" && !value.isEmpty()) {
> 
> Maybe you should have at the top of some file a list of these hard coded strings like
> 
> const String atkSortAttributeName = "sort";
> 
> so that you're not referencing literals

That makes sense. I'll change it.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:86
> > +    if (type == AtkAttributeTypeText) {
> 
> this should be an else if, since the first if does not always return

Ok.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:93
> > +    // Return the passed value unchanged if not filtered.
> 
> this comment seems unnecessary unless you say why you are passing it unfiltered

Ok.

> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:90
> > +    if (type == AtkAttributeTypeText) {
> 
> ditto about else if

Ok.

[EFL EWS Bot]

Comment on attachment 212816 [details]
Patch proposal

Attachment 212816 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2649196

[EFL EWS Bot]

Comment on attachment 212816 [details]
Patch proposal

Attachment 212816 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2651173

[kov's GTK+ EWS bot]

Comment on attachment 212816 [details]
Patch proposal

Attachment 212816 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2651194

[Mario Sanchez Prada]

Created attachment 212974 [details]
Patch proposal

Attaching a new patch addressing the issues pointed out by Chris.

Also, considering now that I'd be adding some constants in AccessibilityUIElementAtk.cpp, I made the most of this patch to move all the static functions there into unnammed namespaces together with those newly defined constants.

[Mario Sanchez Prada]

Created attachment 212986 [details]
Patch proposal

Sorry for the spam but I'm attaching a new version of the previous patch now that I think it's cleaner and more easy to scale, as I put all the attributes related mappings between ATK and layout tests inside a bidimensional array. Also the code in coreAttributeToAtkAttribute() is cleaner too.

We probably should do something similar to simplify the roleToString() funcion, me thinks...

[chris fleizach]

Comment on attachment 212986 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=212986&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:308
> +        String invalidValue = invalidStatus == "spelling" || invalidStatus == "grammar" ? invalidStatus : "true";

I feel like this logic might be better served if it's in WebCore. the platforms shouldn't have to worry about this level of ARIA detail.

I think we should have a new method, isInvalid() that takes care of this logic (since it appears that GTK doesn't want the actual string like Mac does)

[Mario Sanchez Prada]

(In reply to comment #10)
> (From update of attachment 212986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212986&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:308
> > +        String invalidValue = invalidStatus == "spelling" || invalidStatus == "grammar" ? invalidStatus : "true";
> 
> I feel like this logic might be better served if it's in WebCore. the
> platforms shouldn't have to worry about this level of ARIA detail.
> 
> I think we should have a new method, isInvalid() that takes care of this
> logic (since it appears that GTK doesn't want the actual string like Mac does)

Hmm.. actually we want the actual string, but only if it's "true", "grammar" or "spelling". Any other non-empty value that might have been set in the aria-invalid attribute should be returned as "false".

However, that's not what AccessibilityObject::invalidStatus() does:

  const AtomicString& AccessibilityObject::invalidStatus() const
  {
      DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusFalse, ("false", AtomicString::ConstructFromLiteral));
    
      // aria-invalid can return false (default), grammer, spelling, or true.
      const AtomicString& ariaInvalid = getAttribute(aria_invalidAttr);
    
      // If empty or not present, it should return false.
      if (ariaInvalid.isEmpty())
          return invalidStatusFalse;
    
      return ariaInvalid;
  }


... but I'm thinking now that this is actually a bug in there, since the W3C spec is quite clear about that:

 "Any value not recognized in the list of allowed values MUST be treated by user agents as if the value true had been provided"

Thus, I think I will update the patch moving that logic from the ATK wrapper into AccessibilityObject::invalidStatus(). The aria-invalid test should keep passing fine for the Mac as it is, although I will probably add a new element with a not specified value to make sure we return "true" in that case as well.

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 212986 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=212986&action=review
> > 
> > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:308
> > > +        String invalidValue = invalidStatus == "spelling" || invalidStatus == "grammar" ? invalidStatus : "true";
> > 
> > I feel like this logic might be better served if it's in WebCore. the
> > platforms shouldn't have to worry about this level of ARIA detail.
> > 
> > I think we should have a new method, isInvalid() that takes care of this
> > logic (since it appears that GTK doesn't want the actual string like Mac does)
> 
> Hmm.. actually we want the actual string, but only if it's "true", "grammar" or "spelling". Any other non-empty value that might have been set in the aria-invalid attribute should be returned as "false".
> 
> However, that's not what AccessibilityObject::invalidStatus() does:
> 
>   const AtomicString& AccessibilityObject::invalidStatus() const
>   {
>       DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusFalse, ("false", AtomicString::ConstructFromLiteral));
> 
>       // aria-invalid can return false (default), grammer, spelling, or true.
>       const AtomicString& ariaInvalid = getAttribute(aria_invalidAttr);
> 
>       // If empty or not present, it should return false.
>       if (ariaInvalid.isEmpty())
>           return invalidStatusFalse;
> 
>       return ariaInvalid;
>   }
> 
> 
> ... but I'm thinking now that this is actually a bug in there, since the W3C spec is quite clear about that:
> 
>  "Any value not recognized in the list of allowed values MUST be treated by user agents as if the value true had been provided"
> 
> Thus, I think I will update the patch moving that logic from the ATK wrapper into AccessibilityObject::invalidStatus(). The aria-invalid test should keep passing fine for the Mac as it is, although I will probably add a new element with a not specified value to make sure we return "true" in that case as well.

Sounds good

[Mario Sanchez Prada]

Created attachment 213096 [details]
Patch proposal

(In reply to comment #12)
> [...]
> > Thus, I think I will update the patch moving that logic from the ATK
> > wrapper into AccessibilityObject::invalidStatus(). The aria-invalid
> > test should keep passing fine for the Mac as it is, although I will
> > probably add a new element with a not specified value to make sure
> > we return "true" in that case as well.
> 
> Sounds good

Done. See the patch attached. I changed the test to cover all the cases, but it should work on the Mac too.

[chris fleizach]

Comment on attachment 213096 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=213096&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:1348
> +    DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusGrammar, ("grammar", AtomicString::ConstructFromLiteral));

we probably don't need statics for these (as per recent discussions on webkit-dev) - we can probably switch these to just ASCIILiterals inline

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:303
> +    if (!invalidStatus.isEmpty() && invalidStatus != "false") {

invalidStatus should never be empty now right?

[Mario Sanchez Prada]

Comment on attachment 213096 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=213096&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1348
>> +    DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusGrammar, ("grammar", AtomicString::ConstructFromLiteral));
> 
> we probably don't need statics for these (as per recent discussions on webkit-dev) - we can probably switch these to just ASCIILiterals inline

Do you mean also changing the return value of this method from AtomicString& to String?

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:303
>> +    if (!invalidStatus.isEmpty() && invalidStatus != "false") {
> 
> invalidStatus should never be empty now right?

That's very true. I'll change that

[Mario Sanchez Prada]

(In reply to comment #15)
> (From update of attachment 213096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213096&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityObject.cpp:1348
> >> +    DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusGrammar, ("grammar", AtomicString::ConstructFromLiteral));
> > 
> > we probably don't need statics for these (as per recent discussions on webkit-dev) - we can probably switch these to just ASCIILiterals inline
> 
> Do you mean also changing the return value of this method from AtomicString& to String?
> 

Hmm... or maybe you mean using ASCIILiterals just for "grammar" and "spelling" (for the comparison only), and leave the "true" and "false" ones as statically defined AtomicStrings (thus keeping the AtomicString& return type) ?

[chris fleizach]

Comment on attachment 213096 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=213096&action=review

>>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1348
>>>> +    DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusGrammar, ("grammar", AtomicString::ConstructFromLiteral));
>>> 
>>> we probably don't need statics for these (as per recent discussions on webkit-dev) - we can probably switch these to just ASCIILiterals inline
>> 
>> Do you mean also changing the return value of this method from AtomicString& to String?
> 
> Hmm... or maybe you mean using ASCIILiterals just for "grammar" and "spelling" (for the comparison only), and leave the "true" and "false" ones as statically defined AtomicStrings (thus keeping the AtomicString& return type) ?

Ok

[Mario Sanchez Prada]

Created attachment 213168 [details]
Patch proposal

New patch using less static variables and without the empty() check

[Mario Sanchez Prada]

Committed r156835: <http://trac.webkit.org/changeset/156835>