[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 #3 from Mario Sanchez Prada <msanchez at igalia.com>  2012-08-13 11:09:43 PST ---
(From update of attachment 158032)
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.

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