Bug 122015 - [ATK] Expose aria-invalid as a text attribute (not object attribute)
Summary: [ATK] Expose aria-invalid as a text attribute (not object attribute)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-27 09:08 PDT by Mario Sanchez Prada
Modified: 2013-10-03 08:20 PDT (History)
11 users (show)

See Also:


Attachments
Patch proposal (27.87 KB, patch)
2013-09-27 09:34 PDT, Mario Sanchez Prada
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch proposal (40.06 KB, patch)
2013-09-30 03:36 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (39.56 KB, patch)
2013-09-30 06:03 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (46.57 KB, patch)
2013-10-01 10:18 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (46.57 KB, patch)
2013-10-02 09:15 PDT, Mario Sanchez Prada
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2013-09-27 09:08:22 PDT
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
Comment 1 Radar WebKit Bug Importer 2013-09-27 09:08:48 PDT
<rdar://problem/15098367>
Comment 2 Mario Sanchez Prada 2013-09-27 09:34:13 PDT
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.
Comment 3 chris fleizach 2013-09-27 09:44:21 PDT
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
Comment 4 Mario Sanchez Prada 2013-09-27 10:03:27 PDT
(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.
Comment 5 EFL EWS Bot 2013-09-27 10:12:48 PDT
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
Comment 6 EFL EWS Bot 2013-09-27 10:45:32 PDT
Comment on attachment 212816 [details]
Patch proposal

Attachment 212816 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2651173
Comment 7 kov's GTK+ EWS bot 2013-09-27 12:32:31 PDT
Comment on attachment 212816 [details]
Patch proposal

Attachment 212816 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2651194
Comment 8 Mario Sanchez Prada 2013-09-30 03:36:56 PDT
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.
Comment 9 Mario Sanchez Prada 2013-09-30 06:03:19 PDT
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...
Comment 10 chris fleizach 2013-10-01 09:06:22 PDT
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)
Comment 11 Mario Sanchez Prada 2013-10-01 09:44:02 PDT
(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.
Comment 12 chris fleizach 2013-10-01 09:47:43 PDT
(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
Comment 13 Mario Sanchez Prada 2013-10-01 10:18:39 PDT
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.
Comment 14 chris fleizach 2013-10-01 10:21:25 PDT
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?
Comment 15 Mario Sanchez Prada 2013-10-01 13:46:41 PDT
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
Comment 16 Mario Sanchez Prada 2013-10-01 15:29:29 PDT
(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) ?
Comment 17 chris fleizach 2013-10-01 16:28:24 PDT
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
Comment 18 Mario Sanchez Prada 2013-10-02 09:15:10 PDT
Created attachment 213168 [details]
Patch proposal

New patch using less static variables and without the empty() check
Comment 19 Mario Sanchez Prada 2013-10-03 08:20:06 PDT
Committed r156835: <http://trac.webkit.org/changeset/156835>