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.
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. :-)
Created attachment 41846 [details] part 1 and 2 - Fix the states exposed and implement the AtkSelection interface
Can you please split the patches and put them separately for review? Makes things much easier for me.
I mean, since you already had the first one split, I see no reason to not do the second on its own.
(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 on attachment 41817 [details] part 1 - Fix the states exposed Un-obsoleting this one and will then split off the second one.
(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 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.
Created attachment 41852 [details] Just selection
(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.
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.
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 on attachment 41856 [details] Part 1 - Fix the states exposed - Take 2 r=me
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>
All reviewed patches have been landed. Closing bug.
Reopening, still patches to land.
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!
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 on attachment 42068 [details] Part 2 - Selection - Take 3 (remove NULLs) r=me
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 on attachment 42068 [details] Part 2 - Selection - Take 3 (remove NULLs) Try again.
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 on attachment 42068 [details] Part 2 - Selection - Take 3 (remove NULLs) Clearing flags on attachment: 42068 Committed r50282: <http://trac.webkit.org/changeset/50282>
(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.
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 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
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 on attachment 73250 [details] Part 3 - emit 'selected' and 'focused' signals r=me
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>