Bug 25679 – [GTK] Improve accessibility of focusable lists

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Focusable Lists (such as the Version and Component lists in bugzilla) need several improvements in order to be fully accessible:

1. The list itself should implement the accessible selection interface so that ATs can identify which item(s) in the list have be selected. (Please see http://library.gnome.org/devel/atk/unstable/AtkSelection.html for more information.)

2. List items should have the following states:

   a. STATE_SELECTABLE
   b. STATE_SELECTED (as appropriate)
   c. STATE_FOCUSABLE
   d. STATE_FOCUSED (as appropriate)

3. List items should NOT have STATE_EDITABLE as a general rule (but seem to in the current WebKit).

4. The appropriate events should be emitted. Because some lists are multi-selectable, I would suggest:

   a. object:state-changed:focused
   b. object:state-changed:selected

Thus, in normal arrowing about, two events would be emitted -- or potentially three:

   a. A state-changed:focused event (detail1 == 1) for the focused item.

   b. A state-changed:selected event if the item which has focus was selected (detail1 == 1).

   c. A state-changed:focused event (detail1 == 0) for the item which just gave up focus.

If selection for the current item in a multi-selectable list was toggled, a state-changed:selected event would be expected, with detail1 indicating state.

[Joanmarie Diggs]

Created attachment 41817 [details]
part 1 - Fix the states exposed

This patch addresses issues 2 and 3. I'll work on the other issues after I get some sleep. :-)

[Joanmarie Diggs]

Created attachment 41846 [details]
part 1 and 2 - Fix the states exposed and implement the AtkSelection interface

[Xan Lopez]

Can you please split the patches and put them separately for review? Makes things much easier for me.

[Xan Lopez]

I mean, since you already had the first one split, I see no reason to not do the second on its own.

[Joanmarie Diggs]

(In reply to comment #4)
> I mean, since you already had the first one split, I see no reason to not do
> the second on its own.

The first one was quite small so I figured it would be more work as two separate patches, ChangeLogs, commits, etc. But sure... Lemme reboot that box and split them up.

[Joanmarie Diggs]

Comment on attachment 41817 [details]
part 1 - Fix the states exposed

Un-obsoleting this one and will then split off the second one.

[Xan Lopez]

(In reply to comment #5)
> (In reply to comment #4)
> > I mean, since you already had the first one split, I see no reason to not do
> > the second on its own.
> 
> The first one was quite small so I figured it would be more work as two
> separate patches, ChangeLogs, commits, etc. But sure... Lemme reboot that box
> and split them up.

We always prefer to do patches as small as possible, and if you can split a big
task in several small-ish patches that's always the way to go. Not that the
compiler, or the build bots, or anything like that cares, but the human
reviewers do care :)

[Xan Lopez]

Comment on attachment 41817 [details]
part 1 - Fix the states exposed

>  
>  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.

[Joanmarie Diggs]

Created attachment 41852 [details]
Just selection

[Joanmarie Diggs]

(In reply to comment #8)
> (From update of attachment 41817 [details])
> >  
> >  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?

As I recall, without it we were not getting the isSelected() associated with AccessibilityListBoxOption. But I'll double check when WebKit finishes building.

> > +    // 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.

Okie dokie.

[Joanmarie Diggs]

Created attachment 41856 [details]
Part 1 - Fix the states exposed - Take 2

> I don't understand what this cast is for?

I swear there was a good reason for it. But at the moment I couldn't tell you what it is or was. :-) Removed.

> I'd go ahead and add a comment explaining that we don't want this in ATK for
> whatever reason

Done.

[Joanmarie Diggs]

Created attachment 41857 [details]
Part 2 - Selection - Take 2

When splitting the patches, this bit got left out:

@@ -100,6 +101,11 @@ static AccessibilityObject* core(AtkAction* action)
     return core(ATK_OBJECT(action));
 }
 
+static AccessibilityObject* core(AtkSelection* selection)
+{
+    return core(ATK_OBJECT(selection));
+}
+
 static AccessibilityObject* core(AtkText* text)
 {
     return core(ATK_OBJECT(text));

I believe this is now all of the needed bits from Part 2 and none of the bits from Part 1. :-)

[Xan Lopez]

Comment on attachment 41856 [details]
Part 1 - Fix the states exposed - Take 2

r=me

[WebKit Commit Bot]

Comment on attachment 41856 [details]
Part 1 - Fix the states exposed - Take 2

Clearing flags on attachment: 41856

Committed r50054: <http://trac.webkit.org/changeset/50054>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

Reopening, still patches to land.

[Xan Lopez]

Comment on attachment 41857 [details]
Part 2 - Selection - Take 2

Looks good to me, but can you change the NULL for 0 so we can have the bot land it? Thank you!

[Joanmarie Diggs]

Created attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

(In reply to comment #17)
> (From update of attachment 41857 [details])
> Looks good to me, but can you change the NULL for 0 so we can have the bot land
> it? Thank you!

Done (modulo the ones at the end, to be consistent with the surrounding, broken ones). I'll do a de-nitify patch before too long.

Thanks!

[Xan Lopez]

Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

r=me

[WebKit Commit Bot]

Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

Rejecting patch 42068 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11538 test cases.

httpd is already running: pid 46435, killing...
http/tests/appcache/max-size.html -> failed

Exiting early after 1 failures. 8487 tests run.
175.08s total testing time

8486 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

[Xan Lopez]

Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

Try again.

[Eric Seidel (no email)]

Sorry.  The commit queue was down for 8 hours due to bug 30869.  I expect the rejection you saw was due to bug 30098.  I expect to fix both issues today.  My apologies for the delay on landing your patch.  The commit-queue is back up and running: http://webkit-commit-queue.appspot.com/ and your patch should be processed shortly.

[WebKit Commit Bot]

Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

Clearing flags on attachment: 42068

Committed r50282: <http://trac.webkit.org/changeset/50282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joanmarie Diggs]

(In reply to comment #23)
> (From update of attachment 42068 [details])
> Clearing flags on attachment: 42068
> 
> Committed r50282: <http://trac.webkit.org/changeset/50282>

Woo hoo! :-)

Reopening because I still have to cause events to be emitted.

[Mario Sanchez Prada]

Created attachment 72945 [details]
Part 3 - emit 'selected' and 'focused' signals

(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 42068 [details] [details])
> > Clearing flags on attachment: 42068
> > 
> > Committed r50282: <http://trac.webkit.org/changeset/50282>
> 
> Woo hoo! :-)
> 
> Reopening because I still have to cause events to be emitted.

Attaching the last bit to fix this bug, by addressing this pending issue (the events).

Asking for review then...

[chris fleizach]

Comment on attachment 72945 [details]
Part 3 - emit 'selected' and 'focused' signals

View in context: https://bugs.webkit.org/attachment.cgi?id=72945&action=review

> WebCore/ChangeLog:14
> +        'focus-event', along with apropriate detail along with each of

spelling -> apropriate

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:55
> +    AccessibilityRenderObject::AccessibilityChildrenVector items = object->children();

this should be AccessibilityObject::AXChildrenVector

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:56
> +    SelectElement* select = toSelectElement(static_cast<Element*>(object->node()));

you should check if select == 0 after this line

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:58
> +    if (changedItemIndex < 0 || changedItemIndex >= (int)items.size())

use static cast here

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
> +    static RefPtr<AccessibilityObject> oldFocusedObject;

this static should be at the top of the method. i also believe there's a standard way to initialize statics, that you should look for

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:83
> +        g_signal_emit_by_name(axItem, "focus-event", isSelected);

do you want to emit focus-event every time this happens? or do you want to limit it to when axItem != oldFocusedItem

[Mario Sanchez Prada]

Created attachment 73250 [details]
Part 3 - emit 'selected' and 'focused' signals

Thanks for the review, Chris. Now attaching a new patch. See my comments below...

(In reply to comment #27)
> (From update of attachment 72945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72945&action=review
> 
> > WebCore/ChangeLog:14
> > +        'focus-event', along with apropriate detail along with each of
> 
> spelling -> apropriate

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:55
> > +    AccessibilityRenderObject::AccessibilityChildrenVector items = object->children();
> 
> this should be AccessibilityObject::AXChildrenVector

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:56
> > +    SelectElement* select = toSelectElement(static_cast<Element*>(object->node()));
> 
> you should check if select == 0 after this line

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:58
> > +    if (changedItemIndex < 0 || changedItemIndex >= (int)items.size())
> 
> use static cast here

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
> > +    static RefPtr<AccessibilityObject> oldFocusedObject;
> 
> this static should be at the top of the method. 

Fixed.

> i also believe there's a standard way to initialize statics, that you should look for

I guess you mean initializating the var '= 0'. If so -> "Fixed!"

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:83
> > +        g_signal_emit_by_name(axItem, "focus-event", isSelected);
> 
> do you want to emit focus-event every time this happens? or do you want to limit it to when axItem != oldFocusedItem

Yes, that's the expected behavior. The axItem != oldFocusedItem check for the old item is meant to avoid sending the same signal twice in the hypothetical case they were the same object, but for the current one (axItem) we should definitely always emit those signals.

[chris fleizach]

Comment on attachment 73250 [details]
Part 3 - emit 'selected' and 'focused' signals

r=me

[WebKit Commit Bot]

Comment on attachment 73250 [details]
Part 3 - emit 'selected' and 'focused' signals

Clearing flags on attachment: 73250

Committed r71710: <http://trac.webkit.org/changeset/71710>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.