[webkit-reviews] review denied: [Bug 25679] [GTK] Improve accessibility of focusable lists : [Attachment 41817] part 1 - Fix the states exposed

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 26 00:35:30 PDT 2009


Xan Lopez <xan.lopez at gmail.com> has denied Joanmarie Diggs
<joanmarie.diggs at gmail.com>'s request for review:
Bug 25679: [GTK] Improve accessibility of focusable lists
https://bugs.webkit.org/show_bug.cgi?id=25679

Attachment 41817: part 1 - Fix the states exposed
https://bugs.webkit.org/attachment.cgi?id=41817&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>  
>  static void setAtkStateSetFromCoreObject(AccessibilityObject* coreObject,
AtkStateSet* stateSet)
>  {
> -    // Please keep the state list in alphabetical order
> +    AccessibilityObject* parent = coreObject->parentObject();
> +    bool isListBoxOption = parent && parent->isListBox();
> +    if (isListBoxOption)
> +	   coreObject = static_cast<AccessibilityListBoxOption*>(coreObject);

I don't understand what this cast is for?

>  
> +    // Please keep the state list in alphabetical order
>      if (coreObject->isChecked())
>	   atk_state_set_add_state(stateSet, ATK_STATE_CHECKED);
>  
>      // FIXME: isReadOnly does not seem to do the right thing for
>      // controls, so check explicitly for them
> -    if (!coreObject->isReadOnly() ||
> -	   (coreObject->isControl() && coreObject->canSetValueAttribute()))
> +    if ((!coreObject->isReadOnly() ||
> +	   (coreObject->isControl() && coreObject->canSetValueAttribute())) &&
> +	   !isListBoxOption)
>	   atk_state_set_add_state(stateSet, ATK_STATE_EDITABLE);

I'd go ahead and add a comment explaining that we don't want this in ATK for
whatever reason, otherwise the if is impossible to understand.
>  
>      // FIXME: Put both ENABLED and SENSITIVE together here for now
> @@ -408,8 +414,23 @@ static void
setAtkStateSetFromCoreObject(AccessibilityObject* coreObject, AtkSta
>  
>      // TODO: ATK_STATE_SELECTABLE_TEXT
>  
> -    if (coreObject->isSelected())
> +    if (coreObject->canSetSelectedAttribute()) {
> +	   atk_state_set_add_state(stateSet, ATK_STATE_SELECTABLE);
> +	   // Items in focusable lists in Gtk have both STATE_SELECT{ABLE,ED}
> +	   // and STATE_FOCUS{ABLE,ED}. We'll fake the latter based on the
> +	   // former.
> +	   if (isListBoxOption)
> +	       atk_state_set_add_state(stateSet, ATK_STATE_FOCUSABLE);
> +    }
> +
> +    if (coreObject->isSelected()) {
>	   atk_state_set_add_state(stateSet, ATK_STATE_SELECTED);
> +	   // Items in focusable lists in Gtk have both STATE_SELECT{ABLE,ED}
> +	   // and STATE_FOCUS{ABLE,ED}. We'll fake the latter based on the
> +	   // former.
> +	   if (isListBoxOption)
> +	       atk_state_set_add_state(stateSet, ATK_STATE_FOCUSED);
> +    }

Good!

Marking r- to fix those two details.


More information about the webkit-reviews mailing list