[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


Alejandro Piñeiro <apinheiro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159484|0                           |1
        is obsolete|                            |
 Attachment #159484|review?                     |
               Flag|                            |
 Attachment #159646|                            |review?
               Flag|                            |




--- Comment #30 from Alejandro Piñeiro <apinheiro at igalia.com>  2012-08-21 03:42:46 PST ---
Created an attachment (id=159646)
 --> (https://bugs.webkit.org/attachment.cgi?id=159646&action=review)
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.

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