Bug 25679

Summary: [GTK] Improve accessibility of focusable lists
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, commit-queue, eric, mario, walker.willie, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
part 1 - Fix the states exposed
xan.lopez: review-
part 1 and 2 - Fix the states exposed and implement the AtkSelection interface
none
Just selection
none
Part 1 - Fix the states exposed - Take 2
none
Part 2 - Selection - Take 2
xan.lopez: review-
Part 2 - Selection - Take 3 (remove NULLs)
none
Part 3 - emit 'selected' and 'focused' signals
cfleizach: review-
Part 3 - emit 'selected' and 'focused' signals none

Joanmarie Diggs
Reported 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.
Attachments
part 1 - Fix the states exposed (3.89 KB, patch)
2009-10-25 01:40 PDT, Joanmarie Diggs 		xan.lopez: review-
DetailsFormatted DiffDiff
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 		no flags
DetailsFormatted DiffDiff
Just selection (10.30 KB, patch)
2009-10-26 00:40 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Part 1 - Fix the states exposed - Take 2 (3.79 KB, patch)
2009-10-26 02:44 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Part 2 - Selection - Take 2 (10.78 KB, patch)
2009-10-26 03:15 PDT, Joanmarie Diggs 		xan.lopez: review-
DetailsFormatted DiffDiff
Part 2 - Selection - Take 3 (remove NULLs) (10.66 KB, patch)
2009-10-28 16:28 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Part 3 - emit 'selected' and 'focused' signals (6.37 KB, patch)
2010-11-04 08:38 PDT, Mario Sanchez Prada 		cfleizach: review-
DetailsFormatted DiffDiff
Part 3 - emit 'selected' and 'focused' signals (6.54 KB, patch)
2010-11-08 10:46 PST, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Joanmarie Diggs
Comment 1 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. :-)
Joanmarie Diggs
Comment 2 2009-10-25 23:36:17 PDT
Created attachment 41846 [details] part 1 and 2 - Fix the states exposed and implement the AtkSelection interface
Xan Lopez
Comment 3 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.
Xan Lopez
Comment 4 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.
Joanmarie Diggs
Comment 5 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.
Joanmarie Diggs
Comment 6 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.
Xan Lopez
Comment 7 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 :)
Xan Lopez
Comment 8 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.
Joanmarie Diggs
Comment 9 2009-10-26 00:40:25 PDT
Created attachment 41852 [details] Just selection
Joanmarie Diggs
Comment 10 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.
Joanmarie Diggs
Comment 11 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.
Joanmarie Diggs
Comment 12 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. :-)
Xan Lopez
Comment 13 2009-10-26 04:28:18 PDT
Comment on attachment 41856 [details] Part 1 - Fix the states exposed - Take 2 r=me
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2009-10-26 04:42:53 PDT
All reviewed patches have been landed. Closing bug.
Xan Lopez
Comment 16 2009-10-26 04:44:14 PDT
Reopening, still patches to land.
Xan Lopez
Comment 17 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!
Joanmarie Diggs
Comment 18 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!
Xan Lopez
Comment 19 2009-10-28 16:31:07 PDT
Comment on attachment 42068 [details] Part 2 - Selection - Take 3 (remove NULLs) r=me
WebKit Commit Bot
Comment 20 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
Xan Lopez
Comment 21 2009-10-29 06:43:20 PDT
Comment on attachment 42068 [details] Part 2 - Selection - Take 3 (remove NULLs) Try again.
Eric Seidel (no email)
Comment 22 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.
WebKit Commit Bot
Comment 23 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>
WebKit Commit Bot
Comment 24 2009-10-29 10:34:55 PDT
All reviewed patches have been landed. Closing bug.
Joanmarie Diggs
Comment 25 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.
Mario Sanchez Prada
Comment 26 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...
chris fleizach
Comment 27 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
Mario Sanchez Prada
Comment 28 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.
chris fleizach
Comment 29 2010-11-09 14:40:03 PST
Comment on attachment 73250 [details] Part 3 - emit 'selected' and 'focused' signals r=me
WebKit Commit Bot
Comment 30 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>
WebKit Commit Bot
Comment 31 2010-11-09 19:44:59 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.