Bug 25679 - [GTK] Improve accessibility of focusable lists
Summary: [GTK] Improve accessibility of focusable lists
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-10 18:39 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2010-11-09 19:44 PST (History)
6 users (show)

See Also:


Attachments
part 1 - Fix the states exposed (3.89 KB, patch)
2009-10-25 01:40 PDT, Joanmarie Diggs (irc: joanie)
xan.lopez: review-
Details | Formatted Diff | Diff
part 1 and 2 - Fix the states exposed and implement the AtkSelection interface (12.96 KB, patch)
2009-10-25 23:36 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Just selection (10.30 KB, patch)
2009-10-26 00:40 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Part 1 - Fix the states exposed - Take 2 (3.79 KB, patch)
2009-10-26 02:44 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Part 2 - Selection - Take 2 (10.78 KB, patch)
2009-10-26 03:15 PDT, Joanmarie Diggs (irc: joanie)
xan.lopez: review-
Details | Formatted Diff | Diff
Part 2 - Selection - Take 3 (remove NULLs) (10.66 KB, patch)
2009-10-28 16:28 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Part 3 - emit 'selected' and 'focused' signals (6.37 KB, patch)
2010-11-04 08:38 PDT, Mario Sanchez Prada
cfleizach: review-
Details | Formatted Diff | Diff
Part 3 - emit 'selected' and 'focused' signals (6.54 KB, patch)
2010-11-08 10:46 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-05-10 18:39:04 PDT
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.
Comment 1 Joanmarie Diggs (irc: joanie) 2009-10-25 01:40:34 PDT
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. :-)
Comment 2 Joanmarie Diggs (irc: joanie) 2009-10-25 23:36:17 PDT
Created attachment 41846 [details]
part 1 and 2 - Fix the states exposed and implement the AtkSelection interface
Comment 3 Xan Lopez 2009-10-25 23:59:57 PDT
Can you please split the patches and put them separately for review? Makes things much easier for me.
Comment 4 Xan Lopez 2009-10-26 00:00:49 PDT
I mean, since you already had the first one split, I see no reason to not do the second on its own.
Comment 5 Joanmarie Diggs (irc: joanie) 2009-10-26 00:05:57 PDT
(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.
Comment 6 Joanmarie Diggs (irc: joanie) 2009-10-26 00:08:26 PDT
Comment on attachment 41817 [details]
part 1 - Fix the states exposed

Un-obsoleting this one and will then split off the second one.
Comment 7 Xan Lopez 2009-10-26 00:11:47 PDT
(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 :)
Comment 8 Xan Lopez 2009-10-26 00:35:31 PDT
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.
Comment 9 Joanmarie Diggs (irc: joanie) 2009-10-26 00:40:25 PDT
Created attachment 41852 [details]
Just selection
Comment 10 Joanmarie Diggs (irc: joanie) 2009-10-26 00:53:03 PDT
(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.
Comment 11 Joanmarie Diggs (irc: joanie) 2009-10-26 02:44:20 PDT
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.
Comment 12 Joanmarie Diggs (irc: joanie) 2009-10-26 03:15:24 PDT
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. :-)
Comment 13 Xan Lopez 2009-10-26 04:28:18 PDT
Comment on attachment 41856 [details]
Part 1 - Fix the states exposed - Take 2

r=me
Comment 14 WebKit Commit Bot 2009-10-26 04:42:49 PDT
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>
Comment 15 WebKit Commit Bot 2009-10-26 04:42:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Xan Lopez 2009-10-26 04:44:14 PDT
Reopening, still patches to land.
Comment 17 Xan Lopez 2009-10-28 15:35:37 PDT
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!
Comment 18 Joanmarie Diggs (irc: joanie) 2009-10-28 16:28:07 PDT
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!
Comment 19 Xan Lopez 2009-10-28 16:31:07 PDT
Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

r=me
Comment 20 WebKit Commit Bot 2009-10-28 17:04:51 PDT
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
Comment 21 Xan Lopez 2009-10-29 06:43:20 PDT
Comment on attachment 42068 [details]
Part 2 - Selection - Take 3 (remove NULLs)

Try again.
Comment 22 Eric Seidel (no email) 2009-10-29 10:19:33 PDT
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.
Comment 23 WebKit Commit Bot 2009-10-29 10:34:49 PDT
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>
Comment 24 WebKit Commit Bot 2009-10-29 10:34:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Joanmarie Diggs (irc: joanie) 2009-10-29 10:50:54 PDT
(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.
Comment 26 Mario Sanchez Prada 2010-11-04 08:38:54 PDT
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...
Comment 27 chris fleizach 2010-11-07 23:20:06 PST
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
Comment 28 Mario Sanchez Prada 2010-11-08 10:46:45 PST
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.
Comment 29 chris fleizach 2010-11-09 14:40:03 PST
Comment on attachment 73250 [details]
Part 3 - emit 'selected' and 'focused' signals

r=me
Comment 30 WebKit Commit Bot 2010-11-09 19:44:52 PST
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>
Comment 31 WebKit Commit Bot 2010-11-09 19:44:59 PST
All reviewed patches have been landed.  Closing bug.