[webkit-reviews] review canceled: [Bug 73819] [Gtk] Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not ROLE_PUSH_BUTTON : [Attachment 158374] Fixes the bug

[bugzilla-daemon at webkit.org]

Alejandro Piñeiro <apinheiro at igalia.com> has canceled Alejandro Piñeiro
<apinheiro at igalia.com>'s request for review:
Bug 73819: [Gtk] Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not
ROLE_PUSH_BUTTON
https://bugs.webkit.org/show_bug.cgi?id=73819

Attachment 158374: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=158374&action=review

------- Additional Comments from Alejandro Piñeiro <apinheiro at igalia.com>
(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.