[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 #4 from Alejandro Piñeiro <apinheiro at igalia.com>  2012-08-13 11:45:33 PST ---
(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.

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