[Webkit-unassigned] [Bug 73819] [Gtk] Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not ROLE_PUSH_BUTTON

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73819





--- Comment #9 from Mario Sanchez Prada <msanchez at igalia.com>  2012-08-14 10:58:23 PST ---
(From update of attachment 158354)
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 '&&' :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.