[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