Bug 72479 - Accessibility: Multiselect list boxes need to report focus in addition to selection
Summary: Accessibility: Multiselect list boxes need to report focus in addition to sel...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 00:44 PST by Dominic Mazzoni
Modified: 2011-11-25 14:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.77 KB, patch)
2011-11-16 00:53 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (18.89 KB, patch)
2011-11-16 23:14 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (18.96 KB, patch)
2011-11-16 23:38 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (19.00 KB, patch)
2011-11-16 23:49 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (19.25 KB, patch)
2011-11-21 01:29 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2011-11-16 00:44:40 PST
When a user navigates through a multiselect list box using the arrow keys, the accessible representation of the list box and its options must include not only which options are currently selected, but also which one has focus.
Comment 1 Dominic Mazzoni 2011-11-16 00:53:40 PST
Created attachment 115345 [details]
Patch
Comment 2 chris fleizach 2011-11-16 08:09:57 PST
Comment on attachment 115345 [details]
Patch

what platform was this tested on? on the mac, the list box should retain focus() and the selection is returned through selectedChildren(). the concept of selection vs. focus() is different
Comment 3 Dominic Mazzoni 2011-11-16 08:25:01 PST
(In reply to comment #2)
> (From update of attachment 115345 [details])
> what platform was this tested on? on the mac, the list box should retain focus() and the selection is returned through selectedChildren(). the concept of selection vs. focus() is different

This doesn't send any focus events, so I think Safari will be okay - but I'll test some more to make sure.

If you prefer, I could expose this as something other than focus. It's kind of like an active descendant.

Note that on Windows, the focus really does move to the option within the list. So ideally let's create an abstraction that's cross-platform.

- Dominic
Comment 4 chris fleizach 2011-11-16 08:28:51 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 115345 [details] [details])
> > what platform was this tested on? on the mac, the list box should retain focus() and the selection is returned through selectedChildren(). the concept of selection vs. focus() is different
> 
> This doesn't send any focus events, so I think Safari will be okay - but I'll test some more to make sure.
> 
> If you prefer, I could expose this as something other than focus. It's kind of like an active descendant.
> 
> Note that on Windows, the focus really does move to the option within the list. So ideally let's create an abstraction that's cross-platform.
> 
> - Dominic

It seems like this might be a platform decision for windows, i.e.) when asked if an object is isFocused() and the object is a listBoxOptionRole and isSelected() == true, then it returns true.
Comment 5 chris fleizach 2011-11-16 08:32:49 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 115345 [details] [details] [details])
> > > what platform was this tested on? on the mac, the list box should retain focus() and the selection is returned through selectedChildren(). the concept of selection vs. focus() is different
> > 
> > This doesn't send any focus events, so I think Safari will be okay - but I'll test some more to make sure.
> > 
> > If you prefer, I could expose this as something other than focus. It's kind of like an active descendant.
> > 
> > Note that on Windows, the focus really does move to the option within the list. So ideally let's create an abstraction that's cross-platform.
> > 
> > - Dominic
> 
> It seems like this might be a platform decision for windows, i.e.) when asked if an object is isFocused() and the object is a listBoxOptionRole and isSelected() == true, then it returns true.

Does Chrome  move KB focus to each item in the <select> list? The last time I looked at list boxes the options were not actual nodes that could receive focus.. they were literally just strings that were drawn at the right place
Comment 6 Dominic Mazzoni 2011-11-16 17:15:38 PST
> > It seems like this might be a platform decision for windows, i.e.) when asked if an object is isFocused() and the object is a listBoxOptionRole and isSelected() == true, then it returns true.
> 
> Does Chrome  move KB focus to each item in the <select> list? The last time I looked at list boxes the options were not actual nodes that could receive focus.. they were literally just strings that were drawn at the right place

No, keyboard focus stays with the <select> list itself, however there is a concept of the focused element within the list that's not the same as the selection when multiple things are selected. On Windows, the accessible object for the list option actually gets focus, even though it doesn't correspond to a browser element. 

In a list of 5 items, if you start on item 2 and press shift+down, now 2 and 3 are selected, but 3 is focused.  But if you start at 3 and press shift+up, 2 and 3 are selected, but 2 is focused. That's what this changelist is attempting to do - identify which of all of the selected items is the active one.

Alternatives:

1. I could add a new interface isActive that would indicate whether an element is the active descendant of its parent widget.

2. Or, I could have AccessibilityListBox return the active element in activeDescendant(), which is currently only used for ARIA but could be used here as well.
Comment 7 chris fleizach 2011-11-16 17:16:58 PST
(In reply to comment #6)
> > > It seems like this might be a platform decision for windows, i.e.) when asked if an object is isFocused() and the object is a listBoxOptionRole and isSelected() == true, then it returns true.
> > 
> > Does Chrome  move KB focus to each item in the <select> list? The last time I looked at list boxes the options were not actual nodes that could receive focus.. they were literally just strings that were drawn at the right place
> 
> No, keyboard focus stays with the <select> list itself, however there is a concept of the focused element within the list that's not the same as the selection when multiple things are selected. On Windows, the accessible object for the list option actually gets focus, even though it doesn't correspond to a browser element. 
> 
> In a list of 5 items, if you start on item 2 and press shift+down, now 2 and 3 are selected, but 3 is focused.  But if you start at 3 and press shift+up, 2 and 3 are selected, but 2 is focused. That's what this changelist is attempting to do - identify which of all of the selected items is the active one.
> 
> Alternatives:
> 
> 1. I could add a new interface isActive that would indicate whether an element is the active descendant of its parent widget.
> 
> 2. Or, I could have AccessibilityListBox return the active element in activeDescendant(), which is currently only used for ARIA but could be used here as well.

It sounds like this is what isSelected() already does. (as well as selectedChildren())
Comment 8 Dominic Mazzoni 2011-11-16 17:26:53 PST
(In reply to comment #7)
> It sounds like this is what isSelected() already does. (as well as selectedChildren())

Not quite - if multiple items are selected, isSelected / selectedChildren is enough to tell you that several items are selected, but it doesn't tell you what would happen if you were to press shift+up arrow or shift+down arrow to try to extend or contract the selection. You need to know which of those items is the active one.

It's like text selection - it's not enough to know that the selection is from character 5 to character 10, you have to know whether the selection focus is at the beginning or end.
Comment 9 chris fleizach 2011-11-16 17:28:21 PST
(In reply to comment #8)
> (In reply to comment #7)
> > It sounds like this is what isSelected() already does. (as well as selectedChildren())
> 
> Not quite - if multiple items are selected, isSelected / selectedChildren is enough to tell you that several items are selected, but it doesn't tell you what would happen if you were to press shift+up arrow or shift+down arrow to try to extend or contract the selection. You need to know which of those items is the active one.
> 
> It's like text selection - it's not enough to know that the selection is from character 5 to character 10, you have to know whether the selection focus is at the beginning or end.

k, i see. 

i think we should have a new attribute (perhaps isActiveSelection()) for this. it's definitely a different concept than focus and selection

thanks
Comment 10 Dominic Mazzoni 2011-11-16 23:01:55 PST
No problem - I'll switch it to a new attribute, how about isActiveSelectedOption to be a little more clear?
Comment 11 chris fleizach 2011-11-16 23:09:52 PST
(In reply to comment #10)
> No problem - I'll switch it to a new attribute, how about isActiveSelectedOption to be a little more clear?

Does this concept apply to any other widget? if so, we could put it on AccessibilityObject. if not, we could just put it on the list box option
Comment 12 Dominic Mazzoni 2011-11-16 23:14:43 PST
Created attachment 115529 [details]
Patch
Comment 13 WebKit Review Bot 2011-11-16 23:17:18 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 14 chris fleizach 2011-11-16 23:19:54 PST
Comment on attachment 115529 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:93
> +}

extra parens not needed

> Source/WebKit/chromium/src/WebAccessibilityObject.cpp:233
> +

for methods that return a bool, you should use true/false

> LayoutTests/accessibility/multiselect-list-reports-active-option.html:61
> +                layoutTestController.notifyDone();

you need to remove the notificationListener from the object, otherwise notifications can bleed into other tests
Comment 15 Dominic Mazzoni 2011-11-16 23:37:26 PST
Comment on attachment 115529 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:93
>> +}
> 
> extra parens not needed

Done.

>> Source/WebKit/chromium/src/WebAccessibilityObject.cpp:233
>> +
> 
> for methods that return a bool, you should use true/false

Done.

>> LayoutTests/accessibility/multiselect-list-reports-active-option.html:61
>> +                layoutTestController.notifyDone();
> 
> you need to remove the notificationListener from the object, otherwise notifications can bleed into other tests

Thanks, didn't realize that.
Comment 16 Dominic Mazzoni 2011-11-16 23:38:13 PST
Created attachment 115531 [details]
Patch
Comment 17 chris fleizach 2011-11-16 23:40:45 PST
Comment on attachment 115531 [details]
Patch

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

> LayoutTests/accessibility/multiselect-list-reports-active-option.html:23
> +            }

this will mean you'll get one notification and then it will not get any more. i think you want to remove this right before you notifyDone() at the end
Comment 18 Dominic Mazzoni 2011-11-16 23:49:04 PST
Created attachment 115533 [details]
Patch
Comment 19 Dominic Mazzoni 2011-11-16 23:49:32 PST
Comment on attachment 115531 [details]
Patch

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

>> LayoutTests/accessibility/multiselect-list-reports-active-option.html:23
>> +            }
> 
> this will mean you'll get one notification and then it will not get any more. i think you want to remove this right before you notifyDone() at the end

Hmmm, you're right. It looks like the test was still passing for me either way because removeNotificationListener doesn't do anything in the Chromium DumpRenderTree implementation. I can't reproduce the problem where it bleeds into another test, maybe it only affects some platforms?
Comment 20 Dominic Mazzoni 2011-11-18 14:41:28 PST
I updated the patch earlier to put removeNotificationListener in the right place - take another look?
Comment 21 chris fleizach 2011-11-18 14:49:29 PST
Comment on attachment 115533 [details]
Patch

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

looks ok. one minor comment to fix before checking in

> Source/WebCore/accessibility/AccessibilityObject.h:414
>  

I think this should be renamed to

isSelectedOptionActive()

and you should put a comment above to explain what it means
Comment 22 Dominic Mazzoni 2011-11-18 14:53:41 PST
Sounds good, will do, thanks.
Comment 23 Dominic Mazzoni 2011-11-21 01:29:08 PST
Created attachment 116052 [details]
Patch for landing
Comment 24 WebKit Review Bot 2011-11-21 01:50:57 PST
Comment on attachment 116052 [details]
Patch for landing

Clearing flags on attachment: 116052

Committed r100892: <http://trac.webkit.org/changeset/100892>
Comment 25 WebKit Review Bot 2011-11-21 01:51:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Ryosuke Niwa 2011-11-25 14:33:55 PST
The test added by this patch is failing on Mac:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r101180%20(34953)/accessibility/multiselect-list-reports-active-option-actual.txt

To begin with, why did this patch add a test expectation to platform/chromium? That makes very little sense given this is a script test. Are you expecting this test to fail on non-Chromium platforms? If that were the case, then the correct thing to do is to add it to the skipped list and file appropriate bugs for non-Chromium ports. If not, then you should have added the test expectation to LayoutTests/accessibility/