Bug 73819 – Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not ROLE_PUSH_BUTTON

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

The Dojo toggle buttons on the buttons test page [1] currently expose ROLE_PUSH_BUTTON. They should instead expose ROLE_TOGGLE_BUTTON.

[1] http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/form/test_Button.html

[Alejandro Piñeiro]

Created attachment 158032 [details]
Fixes the bug

I added an extra rule on the mapping between WebCore role and ATK roles in order to know if a button is a toggle button. In order to do that I use the attribute aria-pressed:

http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed

Submitting the patch first. I will start the test, but I wanted to give first the patch so it could be reviewed.

PS: the example page has an error. After this patch, the example toggle button that it is initially un-pressed still exposes push-button. This is because initially it doesn't have that attribute set to false. But from the w3c documentation, I understand that that attribute should be there. In the same way, if you press that toggle button, and unpress it again, it properly expose the toggle button role, so the attribute (set to false) is there.

[WebKit Review Bot]

Attachment 158032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/Accessibility..." exit_code: 1
Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:434:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Comment on attachment 158032 [details]
Fixes the bug

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

Thanks for the patch. Overall it looks good, although there are some obvious bits missing for being considered a complete patch:

  - You need to make sure you don't break the coding style. Runnint Tools/Scripts/check-webkit-style is very helpful for this
  - You need to write a Layout test for it (as we spoke about it via IRC)
  - You need to create proper changelog entries (run Tools/Scripts/prepare-ChangeLog from)

You can check http://www.webkit.org/coding/contributing.html for some guidelines on that.

I'll review it informally anyway, just pointing those things out to help you for future iterations of the patch. See my comments below...

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> +    return (!getAttribute(aria_pressedAttr).isEmpty());

This seems to be a little bit too relaxed way to determine that something is a toggle button. I agree with you that probably checking the pressed attribute (and assuming it must be present even if the button is unchecked) is a good idea but I think I'd also add some extra checks to the equation to at least know that we are not doing this over something completely unrelated to a button.

What about something like this?

return (isButton() || ariaRoleAttribute() == ButtonRole)
    && !getAttribute(aria_pressedAttr).isEmpty();

(no extra parenthesis needed, btw)

Still, I'm  a bit concerned of the scenario you mentioned where the 'pressed' attribute would not be there initially. Maybe there's something else we could do that we are missing?

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:436
> +        else

You don't need this 'else' here, as the coding style also tells about it.

[Alejandro Piñeiro]

(In reply to comment #3)
> (From update of attachment 158032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158032&action=review
> 
> Thanks for the patch. 

Thanks for the review.

>Overall it looks good, although there are some obvious bits missing for being considered a complete patch:
> 
>   - You need to make sure you don't break the coding style. Runnint Tools/Scripts/check-webkit-style is very helpful for this
>   - You need to write a Layout test for it (as we spoke about it via IRC)
>   - You need to create proper changelog entries (run Tools/Scripts/prepare-ChangeLog from)
> 
> You can check http://www.webkit.org/coding/contributing.html for some guidelines on that.

Ok, thanks.

> 
> I'll review it informally anyway, just pointing those things out to help you for future iterations of the patch. See my comments below...
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> > +    return (!getAttribute(aria_pressedAttr).isEmpty());
> 
> This seems to be a little bit too relaxed way to determine that something is a toggle button. I agree with you that probably checking the pressed attribute (and assuming it must be present even if the button is unchecked) is a good idea but I think I'd also add some extra checks to the equation to at least know that we are not doing this over something completely unrelated to a button.


Well as we are on the "case ButtonRole:" path, we are doing this to something totally related to a button. 

> What about something like this?
> 
> return (isButton() || ariaRoleAttribute() == ButtonRole)
>     && !getAttribute(aria_pressedAttr).isEmpty();

isButton just checks that roleValue==ButtonRole. So this is equivalent to the case. This seems like check the same twice.

About the aria check, is there any real possibility of a button not being an aria button? There are some extra types of button, but looking at the code, if the aria button role is not a "plain" ButtonRole, it is also the case for the button role. In fact determineAccessibilityRole already checks aria stuff to determine the role. IMHO, having coreObject->roleValue as ButtonRole but the aria role as something different should not happen. Taking that into account, is it still required to add that check there?

> (no extra parenthesis needed, btw)
> 
> Still, I'm  a bit concerned of the scenario you mentioned where the 'pressed' attribute would not be there initially. Maybe there's something else we could do that we are missing?

Well, as I said on my comment, this seems to be an error on Dojo itself. After all, if you press again to unpress the toggle the information is there. The initial state should be the same that the state after press and unpress the toggle button. And as the w3c documentation says:
http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed

"...If the attribute is not present, the button is not a toggle button."

> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:436
> > +        else
> 
> You don't need this 'else' here, as the coding style also tells about it.

Removed, I will upload a new patch soon.

[Alejandro Piñeiro]

(In reply to comment #4)

> 
> > What about something like this?
> > 
> > return (isButton() || ariaRoleAttribute() == ButtonRole)
> >     && !getAttribute(aria_pressedAttr).isEmpty();
> 
> isButton just checks that roleValue==ButtonRole. So this is equivalent to the case. This seems like check the same twice.
> 
> About the aria check, is there any real possibility of a button not being an aria button? There are some extra types of button, but looking at the code, if the aria button role is not a "plain" ButtonRole, it is also the case for the button role. In fact determineAccessibilityRole already checks aria stuff to determine the role. IMHO, having coreObject->roleValue as ButtonRole but the aria role as something different should not happen. Taking that into account, is it still required to add that check there?

Sorry I have just realized that you are talking about the new method created at AccessibilityRenderObject. It is true that in this case it was already checked that the role is button, but if in the future someone wants to add that method, that method should be safe by his own. I will also add those checks.

Sorry for the noise.

[Mario Sanchez Prada]

(In reply to comment #5)
> > [...]
> > About the aria check, is there any real possibility of a button not being an aria button? There are some extra 
> > types of button, but looking at the code, if the aria button role is not a "plain" ButtonRole, it is also the 
> > case for the button role. In fact determineAccessibilityRole already checks aria stuff to determine the role. 
> > IMHO, having coreObject->roleValue as ButtonRole but the aria role as something different should not 
> > happen. Taking that into account, is it still required to add that check there?
> 
> Sorry I have just realized that you are talking about the new method created at AccessibilityRenderObject. 
> It is true that in this case it was already checked that the role is button, but if in the future someone wants 
> to add that method, that method should be safe by his own. I will also add those checks.

Yes, exactly: as you're implementing a new method isToggleButton() in AccessibilityObject / AccessibilityRenderObject, that should be agnostic to any other check you might do in the callee, and therefore able to determine whether it's a toggle button by itself.

Maybe you could do, to avoid checking things twice, change the code in the wrapper to somthing like this:

@@ -538,7 +543,7 @@ static AtkRole webkitAccessibleGetRole(AtkObject* object)
     if (coreObject->isPasswordField())
         return ATK_ROLE_PASSWORD_TEXT;
+ 
+   if (coreObject->isToggleButton())
+        return ATK_ROLE_PUSH_BUTTON;

     return atkRole(coreObject);
 }

... and you leave atkRole(AccessibilityRole role) untouched (thus, receiving an AccessibilityRole as it used to do). Perhaps it would make sense to rename atkRole() to atkRoleForCoreRole(), or something like that, to make clear the purpose of the method.

I personally think this way is cleaner. What do you think?

> Sorry for the noise.

No problem. Happy to help

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
>      return atkRole(coreObject);
                 ^^^^^^^^^^^^^
I obviously meant atkRole(coreObject->roleValue()) :-)

[Alejandro Piñeiro]

Created attachment 158354 [details]
Fixes the  bug

Updated patch, with all the suggestion made by Mario Sánchez and adding a layout test.

[Mario Sanchez Prada]

Comment on attachment 158354 [details]
Fixes the  bug

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

Thanks for the patch, Alejandro Piñeiro. The patch looks good to me with the exception of some nits and a small mistake.

See my comments below...

> LayoutTests/ChangeLog:10
> +        ATK_ROLE_TOGGLE_BUTTON when required

Nit. Missing period at the end of the line.

> Source/WebCore/ChangeLog:10
> +        ATK_ROLE_TOGGLE_BUTTON when required

Same nit here :)

> Source/WebCore/ChangeLog:22
> +        (webkitAccessibleGetRole):

There has been a recent discussion lately in the mailing list on the convenience of documenting changes for every function in the ChangeLog, and the general agreement seems to be that at least a brief description should be included.

So, it would be nice if you could add a brief description of the changes done in each function.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> +    return (isButton() || ariaRoleAttribute()  == ButtonRole) &

There's an extra space here (before the '=='). Also, you're using a bit-wise operation with '&', when I think you meant the boolean operator '&&' :-)

[Alejandro Piñeiro]

Created attachment 158374 [details]
Fixes the bug

Update after review on comment 9

[Mario Sanchez Prada]

The patch looks perfect to me now. Putting Chris on CC as he's a True Reviewer

[WebKit Review Bot]

Attachment 158374 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 158374 [details]
Fixes the bug

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:494
> +        !getAttribute(aria_pressedAttr).isEmpty();

need to fix style warning

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:495
> +}

I think isButton() already returns true if ariaRoleAttribute is button.

You should probably have a comment explaining why having a non-empty aria-pressed attribute means its a toggle button

> Source/WebCore/accessibility/AccessibilityRenderObject.h:74
> +    virtual bool isToggleButton() const;

this should probably be private

[Alejandro Piñeiro]

Created attachment 158537 [details]
Fixes the bug

(In reply to comment #13)
> (From update of attachment 158374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158374&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:494
> > +        !getAttribute(aria_pressedAttr).isEmpty();
> 
> need to fix style warning

Fixed


> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:495
> > +}
> 
> I think isButton() already returns true if ariaRoleAttribute is button.
> 
> You should probably have a comment explaining why having a non-empty aria-pressed attribute means its a toggle button

Added

> > Source/WebCore/accessibility/AccessibilityRenderObject.h:74
> > +    virtual bool isToggleButton() const;
> 
> this should probably be private

Initially I added that method as an utility method on WebKitAccessibleWrapperAtk. Then I decided that could be useful in general, so I moved it to AccessibilityObject. But if I set it as private, I can't use it at WebKitAccessibleWrapperAtk. So in this patch it is still public. If this is a problem, I could remove it from AccessibilityObject and AccessibilityRenderObject and implement it at WebkitAccessibleWrapperAtk.

[chris fleizach]

(In reply to comment #14)
> 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:495
> > > +}
> > 
> > I think isButton() already returns true if ariaRoleAttribute is button.

What about this comment?

> 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.h:74
> > > +    virtual bool isToggleButton() const;
> > 
> > this should probably be private
> 

I think you wan to leave it public on AccessibilityObject, but make it private on AccessibilityRenderObject.

[Alejandro Piñeiro]

Created attachment 158769 [details]
Fixes the bug

(In reply to comment #15)
> (In reply to comment #14)
> > 
> > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:495
> > > > +}
> > > 
> > > I think isButton() already returns true if ariaRoleAttribute is button.
> 
> What about this comment?

True, I have simplified the check on the fuction.

> > > > Source/WebCore/accessibility/AccessibilityRenderObject.h:74
> > > > +    virtual bool isToggleButton() const;
> > > 
> > > this should probably be private
> > 
> 
> I think you wan to leave it public on AccessibilityObject, but make it private on AccessibilityRenderObject.

Ok. Now isToggleButton is public on AccessibilityObject but private on AccessibilityRenderObject.

[chris fleizach]

Comment on attachment 158769 [details]
Fixes the bug

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

after thinking about this patch more, I think we want to add a new WebCore accessibility role, and then just change the GTK mapping so the ToggleButtonRole (WebCore) maps to ATK_ROLE_TOGGLE_BUTTON
On the other platforms, it should still map to native button role. 

This will allow other platforms to identify toggle buttons in the same manner if they need to, without realizing they need to check if something is a toggleButton() after the fact

Similarly, I feel like these likes that I see in the AXWrapperAtk.cpp class can be removed and you can rely on atkRole to return the right thing, because PasswordField should be a WebCore role

if (coreObject->isPasswordField())
539539        return ATK_ROLE_PASSWORD_TEXT;

> LayoutTests/ChangeLog:8
> +        Added a extra rule on the mapping of WebCore accessibility roles

Comment should say

"Added a test to verify that when aria-pressed is present, buttons will have the appropriate role"

> LayoutTests/platform/gtk/accessibility/aria-toggle-button-role.html:17
> +    obj2 = accessibilityController.focusedElement;

You should also test the case when aria-pressed is empty here.

and you should test the case when aria-pressed is on an non-button role

> Source/WebCore/ChangeLog:10
> +        ATK_ROLE_TOGGLE_BUTTON when required.

Comment should say:
"Added an extra rule to the ATK accessibility role mappings based on whether aria-pressed is present.
http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> +    // From http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed

Comment should say

"If aria-pressed is present, then it should be exposed as a toggle button.
http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"

[Alejandro Piñeiro]

(In reply to comment #17)
> (From update of attachment 158769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158769&action=review
> 
> after thinking about this patch more, I think we want to add a new WebCore accessibility role, and then just change the GTK mapping so the ToggleButtonRole (WebCore) maps to ATK_ROLE_TOGGLE_BUTTON
> On the other platforms, it should still map to native button role. 

I'm not sure if I follow this "On the other platforms" sentence. What AccessibilityObject::roleValue should return in the case of being a toggle button (so the value of m_role computed at determineAccessibilityNode)?  If it always returns the new ToggleButtonRole, it will affect the other platforms.

One option would be use some #if PLATFORM(GTK) on determineAccessibilityRole, in order to return ToggleButtonRole only on gtk platform. But in that case I don't see the need to add a new role.

> This will allow other platforms to identify toggle buttons in the same manner if they need to, without realizing they need to check if something is a toggleButton() after the fact

See my previous questions. I fear that suddenly returning ToggleButtonRole for all platforms will somehow break those platforms, as they would be expecting a ButtonRole and suddenly they will receive a ToggleButtonRole

> 
> Similarly, I feel like these likes that I see in the AXWrapperAtk.cpp class can be removed and you can rely on atkRole to return the right thing, because PasswordField should be a WebCore role
> 
> if (coreObject->isPasswordField())
> 539539        return ATK_ROLE_PASSWORD_TEXT;

Well, IMHO, it shouldn't. On ATK we plan to stop to represent password fields with roles, and use instead states. The summary is that we found that text entries are not the only case that would require a "password mode". And after all being a password entry is just a custom state, where the entry are not showing their content (custom type of visibility). More info on this bug:
https://bugzilla.gnome.org/show_bug.cgi?id=668025#c1

> > LayoutTests/ChangeLog:8
> > +        Added a extra rule on the mapping of WebCore accessibility roles
> 
> Comment should say
> 
> "Added a test to verify that when aria-pressed is present, buttons will have the appropriate role"

Ok

> > LayoutTests/platform/gtk/accessibility/aria-toggle-button-role.html:17
> > +    obj2 = accessibilityController.focusedElement;
> 
> You should also test the case when aria-pressed is empty here.
> 
> and you should test the case when aria-pressed is on an non-button role

Ok

> > Source/WebCore/ChangeLog:10
> > +        ATK_ROLE_TOGGLE_BUTTON when required.
> 
> Comment should say:
> "Added an extra rule to the ATK accessibility role mappings based on whether aria-pressed is present.
> http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"

Ok

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> > +    // From http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed
> 
> Comment should say
> 
> "If aria-pressed is present, then it should be exposed as a toggle button.
> http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"

Ok

[chris fleizach]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 158769 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158769&action=review
> > 
> > after thinking about this patch more, I think we want to add a new WebCore accessibility role, and then just change the GTK mapping so the ToggleButtonRole (WebCore) maps to ATK_ROLE_TOGGLE_BUTTON
> > On the other platforms, it should still map to native button role. 
> 
> I'm not sure if I follow this "On the other platforms" sentence. What AccessibilityObject::roleValue should return in the case of being a toggle button (so the value of m_role computed at determineAccessibilityNode)?  If it always returns the new ToggleButtonRole, it will affect the other platforms.

You will need to add mappings on the other platforms so they map ToggleButton to whatever ButtonRole points to


> 
> One option would be use some #if PLATFORM(GTK) on determineAccessibilityRole, in order to return ToggleButtonRole only on gtk platform. But in that case I don't see the need to add a new role.
> 
> > This will allow other platforms to identify toggle buttons in the same manner if they need to, without realizing they need to check if something is a toggleButton() after the fact
> 
> See my previous questions. I fear that suddenly returning ToggleButtonRole for all platforms will somehow break those platforms, as they would be expecting a ButtonRole and suddenly they will receive a ToggleButtonRole
> 
> > 
> > Similarly, I feel like these likes that I see in the AXWrapperAtk.cpp class can be removed and you can rely on atkRole to return the right thing, because PasswordField should be a WebCore role
> > 
> > if (coreObject->isPasswordField())
> > 539539        return ATK_ROLE_PASSWORD_TEXT;
> 
> Well, IMHO, it shouldn't. On ATK we plan to stop to represent password fields with roles, and use instead states. The summary is that we found that text entries are not the only case that would require a "password mode". And after all being a password entry is just a custom state, where the entry are not showing their content (custom type of visibility). More info on this bug:
> https://bugzilla.gnome.org/show_bug.cgi?id=668025#c1
> 
> > > LayoutTests/ChangeLog:8
> > > +        Added a extra rule on the mapping of WebCore accessibility roles
> > 
> > Comment should say
> > 
> > "Added a test to verify that when aria-pressed is present, buttons will have the appropriate role"
> 
> Ok
> 
> > > LayoutTests/platform/gtk/accessibility/aria-toggle-button-role.html:17
> > > +    obj2 = accessibilityController.focusedElement;
> > 
> > You should also test the case when aria-pressed is empty here.
> > 
> > and you should test the case when aria-pressed is on an non-button role
> 
> Ok
> 
> > > Source/WebCore/ChangeLog:10
> > > +        ATK_ROLE_TOGGLE_BUTTON when required.
> > 
> > Comment should say:
> > "Added an extra rule to the ATK accessibility role mappings based on whether aria-pressed is present.
> > http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"
> 
> Ok
> 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:493
> > > +    // From http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed
> > 
> > Comment should say
> > 
> > "If aria-pressed is present, then it should be exposed as a toggle button.
> > http://www.w3.org/TR/wai-aria/states_and_properties#aria-pressed"
> 
> Ok

[Alejandro Piñeiro]

Created attachment 159127 [details]
Fixes the bug

Updated patch based on the last comments. It adds a new role to WebCore: ToggleButtonRole. On gtk platform the mapping is ToggleButtonRole->ATK_ROLE_TOGGLE_BUTTON. On mac platform the mapping is ToggleButtonRole->NSAccessibilityButtonRole. The rest of the platforms doesn't have any role mapping.

[Peter Beverloo (cr-android ews)]

Comment on attachment 159127 [details]
Fixes the bug

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

[WebKit Review Bot]

Comment on attachment 159127 [details]
Fixes the bug

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

[Alejandro Piñeiro]

Created attachment 159393 [details]
Fixes the bug

This last update adds chromium platform specifics. I didn't add it on the previous one because I only searched platform specific code on WebCore, as with gtk and mac, but it seems that most specific accessibility related code for chromium resides at WebKit level.

As with mac, I tried to make a ToggleButton->Button role matching, but I was not able to do that. This is because chromium WebAccessibilityRole needs to be a perfect 1-to-1 match with WebCore AccessibilityRole. In fact, at WebAccessibilityObject::roleValue, the matching rule is a static_cast of the AccessibilityObject wrapped.

So in the end I needed to add a new WebAccessibilityRole, WebAccessibilityRoleToggleButton. I also needed to update AccessibilityUIElementChromium in order to print the proper role name.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dominic Mazzoni]

As you discovered, the platform-specific role mappings in Chromium aren't done inside WebKit - we pass the cross-platform data structure to the browser process and do the platform-native mapping there.

Thanks for the heads-up. I'll add a mapping for WebAccessibilityRoleToggleButton in the Chromium codebase after this lands.

[chris fleizach]

Comment on attachment 159393 [details]
Fixes the bug

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:356
>          role = PopUpButtonRole;

It looks like this logic is duplicated 3 times (and seems likely to grow)
we should probably add a method (something like)

AccessibilityRole buttonRoleType() const;  

that will check ariaPressed() and ariaHasPopup()

> Source/WebCore/accessibility/AccessibilityObject.h:455
> +    virtual bool hasARIAPressed() const { return false; }

I don't know which naming is better (ariaHas or hasARIA), but we should probably stick with the precedent here (so maybe use ariaHasPressedState())

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1689
> +bool AccessibilityRenderObject::hasARIAPressed() const

this method should be in AccessibilityObject.cpp

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3148
> +        return ButtonRole;

another place to use that new method

[Alejandro Piñeiro]

(In reply to comment #26)
> (From update of attachment 159393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159393&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:356
> >          role = PopUpButtonRole;
> 
> It looks like this logic is duplicated 3 times (and seems likely to grow)
> we should probably add a method (something like)
> 
> AccessibilityRole buttonRoleType() const;  
> 
> that will check ariaPressed() and ariaHasPopup()

Ok.


> > Source/WebCore/accessibility/AccessibilityObject.h:455
> > +    virtual bool hasARIAPressed() const { return false; }
> 
> I don't know which naming is better (ariaHas or hasARIA), but we should probably stick with the precedent here (so maybe use ariaHasPressedState())

I guess that that precedent is ariaHasPopup. I was tempted to use this nomemclature, but looking at the code the meaning of both methods are different:

bool AccessibilityRenderObject::ariaHasPopup() const
{
    return elementAttributeValue(aria_haspopupAttr);
}

ariasHasPopup is looking for the attribute has-popup [1], and returns its value. The method that I has just added checks if the object has or not (the value is irrelevant) the pressed state. So in one case this "has" is part of the name of the attribute and in the other case is not. 

But it is true that this "has" thing can be confusing. What about something like "ariaPressedAttrIsPresent" or just "ariaPressedIsPressent"?


> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1689
> > +bool AccessibilityRenderObject::hasARIAPressed() const
> 
> this method should be in AccessibilityObject.cpp
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3148
> > +        return ButtonRole;
> 
> another place to use that new method

Ok

[1] http://www.w3.org/TR/wai-aria/states_and_properties#aria-haspopup

[Alejandro Piñeiro]

Created attachment 159484 [details]
Fixes the bug

Updated patch based on comment 26 review.

I didn't add a ariaHasPressedState as suggested, because as I explain in comment 27, this is confusing with ariaHasPopup(). The method I'm adding checks if an attribute is present, ariaHasPopup checks if has-popup attribute is true.

So I added a method called ariaPressedIsPresent (I felt that ariaPressedAttributeIsPresent was too long).

[chris fleizach]

Comment on attachment 159484 [details]
Fixes the bug

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

looking good, besides minor nits. i think you might want to wait for someone on the google CC list to review the chromium change, although I'm sure it will be fine

> Source/WebCore/accessibility/AccessibilityObject.cpp:1743
> +// This method assumes that the object is already a button-related

I think this comment is probably not needed. it just explains what the code does, which is pretty easy to read.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1750
> +    // If aria-pressed is present, then it should be exposed as a toggle button.

you could ASSERT(isButton()) and return UnknownRole if not a button

[Alejandro Piñeiro]

Created attachment 159646 [details]
Fixes the bug

(In reply to comment #29)
> (From update of attachment 159484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159484&action=review
> 
> looking good, besides minor nits. i think you might want to wait for someone on the google CC list to review the chromium change, although I'm sure it will be fine

They were automatically added on comment 24, and I added Dominic Mazzoni to the CC list

Dominic could you review the patch to check if it is ok with you?

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1743
> > +// This method assumes that the object is already a button-related
> 
> I think this comment is probably not needed. it just explains what the code does, which is pretty easy to read.

Done.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1750
> > +    // If aria-pressed is present, then it should be exposed as a toggle button.
> 
> you could ASSERT(isButton()) and return UnknownRole if not a button

I think that I can't. We have a chicken-egg problem here. isButton() just checks if roleValue() == ButtonRole. roleValue just returns m_role. m_role is computed at determineAccessibilityRole. And this new function (buttonRoleType) is used as an auxiliar function at determineAccessibilityRole. So, when buttonRoleType is called you are still computing m_role, so isButton() return value could be wrong.

This is the reason I added that comment "This method assumes that the object is already a button-related role object". You should already know that it is a button role, for example that the object has the tag name buttonTag.

Taking into account that this is an auxiliary method, and that you need to take that into account, I don't see this method really suitable for a public method. I have moved it to the protected section.

[Dominic Mazzoni]

Looks good for google/chromium - no need to wait for an additional review. Thanks.

[Mario Sanchez Prada]

Comment on attachment 159646 [details]
Fixes the bug

Setting the cq+ flag then

[WebKit Review Bot]

Comment on attachment 159646 [details]
Fixes the bug

Rejecting attachment 159646 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
/webkit-commit-queue/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 76, in run
    decode_output=decode_output)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 407, in run_command
    output = output.decode(self._child_process_encoding())
  File "/usr/lib/python2.6/encodings/utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode bytes in position 231-234: invalid data

Full output: http://queues.webkit.org/results/13567160

[Alejandro Piñeiro]

Created attachment 159882 [details]
Fixes the bug

For any reason, I correctly saved three of the ChangeLog in a utf-8 format (see my surname), but one was saved in a different encoding (when I didn't use any different program or command to save it).

This patch is the same that the previous one, but utf-8 compliant.

[WebKit Review Bot]

Comment on attachment 159882 [details]
Fixes the bug

Clearing flags on attachment: 159882

Committed r126369: <http://trac.webkit.org/changeset/126369>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Beverloo]

Since this change isn't limited to GTK code, please don't add the [Gtk] prefix to the bug title. In general, platform prefixes should only be used if the change is isolated the that platform's codebase, and doesn't touch any other code.

[Alejandro Piñeiro]

(In reply to comment #37)
> Since this change isn't limited to GTK code, please don't add the [Gtk] prefix to the bug title. In general, platform prefixes should only be used if the change is isolated the that platform's codebase, and doesn't touch any other code.

Initially this bug was a platform specific bug. First 5 patches were only affecting GTK platform. The rationale was that the GTK platform already has the toggle button role (ATK_ROLE_TOGGLE_BUTTON), and as the other platforms didn't have it durin, it was just about adding a mapping on gtk platform.

But on comment 17 Chris Fleizach suggested that it would be good to add that role to WebCore. It is supposed that at that moment I should have changed the title of the bug?

BTW: I don't have the permissions to change the title of the bug right now.