Bug 73819

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.

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.

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.

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

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

(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

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

Created attachment 158354 [details] Fixes the bug Updated patch, with all the suggestion made by Mario Sánchez and adding a layout test.

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 '&&' :-)

Created attachment 158374 [details] Fixes the bug Update after review on comment 9

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

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.

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

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.

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

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.

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"

(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

(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

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.

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

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

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.

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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.

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

(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

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

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

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.

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

Comment on attachment 159646 [details] Fixes the bug Setting the cq+ flag then

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

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.

Comment on attachment 159882 [details] Fixes the bug Clearing flags on attachment: 159882 Committed r126369: <http://trac.webkit.org/changeset/126369>

All reviewed patches have been landed. Closing bug.

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.

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