379678 - HTML combo box a11y cleanup

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070503 Minefield/3.0a5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070503 Minefield/3.0a5pre

Beginning with the 4/26 build (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070426 Minefield/3.0a4pre) HTML combo boxes changed.  They used to have a single child (menu) whose children were menu items.  Now they have a single child (still a menu) whose children are list items. 

As menu items they were more consistent with the contents of lists (of the form field variety) whose children are also menu items.  Now, the selectable items within a combo box share the same rolename as the children of bulleted/numbered lists, even though they are rather different creatures.  

Reproducible: Always

Steps to Reproduce:
1. Navigate to a page that contains combo boxes 
2. Examine the combo box using Accerciser

Actual Results:  
The contents of the combo box have the rolename of list item.

Expected Results:  
The contents of the combo box would have the rolename of menu item.

Comment 1

[Aaron Leventhal]

I'm going to broaden the scope of this bug, because I've found many issues with the HTML combos, some of which affect access with JAWS.

Comment 2

[Aaron Leventhal]

Attachment: Fix roles, states and events

This fixes several JAWS bugs with Firefox.

Comment 3

[alexander :surkov (:asurkov)]

Is there a reason to combine patches of this bug and bug 376887?

Comment 4

[Aaron Leventhal]

Attachment: Correct patch

Yes, tired-ness.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 263696 [details] [diff] [review]
Correct patch


>-  nsPoint frameOffset;
>   nsIView *containingView = frame->GetViewExternal();
>-  if (!containingView) {
>+  if (containingView) {
>+    relFrameRect.x = relFrameRect.y = 0;

Why? Isn't it suitable to use just GetOffsetFromView() here?

> void nsDocAccessible::CharacterDataChanged(nsIDocument *aDocument,
>                                            nsIContent* aContent,
>                                            CharacterDataChangeInfo* aInfo)
> {
>-  InvalidateCacheSubtree(aContent, nsIAccessibleEvent::EVENT_REORDER);
>+  // XXX fire text change events here? See bug 377891

What about MSAA?

>-  if (comboFrame && comboFrame->IsDroppedDown())
>+  if (comboFrame && comboFrame->IsDroppedDown()) {
>     *aState |= nsIAccessibleStates::STATE_EXPANDED;
>-  else
>+  }
>+  else {
>+    *aState &= ~nsIAccessibleStates::STATE_FOCUSED; // Focus is on an option

Why does Gecko said it is focused, but we say no? Is it a difference between gecko and AT?

>   nsIComboboxControlFrame* comboFrame = nsnull;
>   boundsFrame->QueryInterface(NS_GET_IID(nsIComboboxControlFrame), (void**)&comboFrame);
>-  if (!comboFrame)
>-    return NS_ERROR_FAILURE;

Why it's not critical any more?

>-  if (comboFrame->IsDroppedDown())
>-    *aState |= nsIAccessibleStates::STATE_FLOATING |
>-               nsIAccessibleStates::STATE_FOCUSABLE;

I assume dropdown list can't be focusable, only dropdown list items, correct?

>-      if (frame)
>-        hWnd = (HWND)frame->GetWindow()->GetNativeData(NS_NATIVE_WINDOW);
>+      if (frame) {
>+        nsIWidget *window = frame->GetWindow();
>+        PRBool isVisible;
>+        window->IsVisible(isVisible);
>+        if (isVisible) {
>+          // If not visible use containing documen's window, because events
>+          // in hidden windows are often ignored by JAWS
>+          // Typical example: focus events on options in a collapsed combo box

I'm confused by words JAWS (what about windows eyes?) and often (not always?). And then if they are ignored what we shouldn't fire them?

Comment 6

[Aaron Leventhal]

> Why? Isn't it suitable to use just GetOffsetFromView() here?
Because that gets 1 view higher up in the parent chain, when the current frame already has a view.

>>-  InvalidateCacheSubtree(aContent, nsIAccessibleEvent::EVENT_REORDER);
>>+  // XXX fire text change events here? See bug 377891
> What about MSAA?
MSAA could get a NAMECHANGE event, but no one is listening for those. I think if we do something for IA2/ATK that's enough.

> Why does Gecko said it is focused, but we say no? Is it a difference between
> gecko and AT?
The higher up code is just treating a dropdown like a regular listbox. But, a dropdown cannot itself receive focus.

> Why it's not critical any more?
Because it just means the dropdown frame wasn't created yet -- it has never been dropped down so it wasn't needed.

> I assume dropdown list can't be focusable, only dropdown list items, correct?
Right.

>>+          // If not visible use containing documen's window, because events
>>+          // in hidden windows are often ignored by JAWS
>>+          // Typical example: focus events on options in a collapsed combo box
> I'm confused by words JAWS (what about windows eyes?) and often (not always?).
> And then if they are ignored what we shouldn't fire them?
This is just to fix combo boxes with JAWS. Window-Eyes already worked with combo boxes because they use the value change event in the closed combo box case.
JAWS will only pay attention to the focus events on the list items. We can't get Freedom Scientific to fix that, so we'll use the focus events. However, JAWS will ignore events on a hidden window. So, in order to fix the bug where JAWS doesn't echo the focus events in a closed combo box, we need to use an HWND for a visible window.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> > Why? Isn't it suitable to use just GetOffsetFromView() here?
> Because that gets 1 view higher up in the parent chain, when the current frame
> already has a view.

What about GetClosestView()?

> > Why does Gecko said it is focused, but we say no? Is it a difference between
> > gecko and AT?
> The higher up code is just treating a dropdown like a regular listbox. But, a
> dropdown cannot itself receive focus.

It's combobox accessible not dropdown list. Or?

> >>+          // If not visible use containing documen's window, because events
> >>+          // in hidden windows are often ignored by JAWS
> >>+          // Typical example: focus events on options in a collapsed combo box
> > I'm confused by words JAWS (what about windows eyes?) and often (not always?).
> > And then if they are ignored what we shouldn't fire them?
> This is just to fix combo boxes with JAWS. Window-Eyes already worked with
> combo boxes because they use the value change event in the closed combo box
> case.
> JAWS will only pay attention to the focus events on the list items. We can't
> get Freedom Scientific to fix that, so we'll use the focus events. However,
> JAWS will ignore events on a hidden window. So, in order to fix the bug where
> JAWS doesn't echo the focus events in a closed combo box, we need to use an
> HWND for a visible window.
> 

Extended commnet would be excellent like you shown above :)

Comment 8

[Aaron Leventhal]

> What about GetClosestView()?
How does that improve the situation? Can you show me the recommended lines of code? I can see if it works.

> It's combobox accessible not dropdown list. Or?
Sorry, I answered the wrong question.
It is a difference in MSAA focus. In Gecko, the combo box itself has focus. In MSAA, the option has focus. We would not have to do this if JAWS listened to value change events.

I'll fix the comment about the hidden window.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> > What about GetClosestView()?
> How does that improve the situation? Can you show me the recommended lines of
> code? I can see if it works.

Probably
nsRect relFrameRect = frame->GetRect();
nsPoint frameOffset;
nsIView *closestView = frame->GetClosestView(&frameOffset);
if (!closestView)
  return PR_FALSE; // no view -- not visible
relFrameRect.MoveTo(frameOffset);

> > It's combobox accessible not dropdown list. Or?
> Sorry, I answered the wrong question.
> It is a difference in MSAA focus. In Gecko, the combo box itself has focus. In
> MSAA, the option has focus. We would not have to do this if JAWS listened to
> value change events.

it makes sense for atk too, right?

> I'll fix the comment about the hidden window.
> 

Thank you.

Comment 10

[Aaron Leventhal]

I'll test the patch with Orca and LSR to make sure it's good for ATK. I'll also test with Window-Eyes.

Comment 11

[Aaron Leventhal]

Attachment: Fixed comment

I couldn't use GetClosestView() because it's not virtual and is therefore not available outside of layout right now. It doesn't seem worth changing that.

Also, I tested with Window-Eyes, LSR and Orca. None of them were affected by this patch.

Comment 12

[Aaron Leventhal]

Attachment: Also fix OFFSCREEN/SHOWING for list items (bug 366340)

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 263930 [details] [diff] [review]
Also fix OFFSCREEN/SHOWING for list items (bug 366340)

r=me with the following nits:

>   nsRect relFrameRect = frame->GetRect();
>-  nsPoint frameOffset;
>   nsIView *containingView = frame->GetViewExternal();
>-  if (!containingView) {
>+  if (containingView) {
>+    relFrameRect.x = relFrameRect.y = 0;

I would like to see comment why frame x,y are not suitable for visibility check.

> 
>   // remove STATE_SHOWING if parent is STATE_COLLAPSED

Gecko hasn't STATE_SHOWING state.

Comment 14

[Joanmarie Diggs]

Hi Aaron.  Combo boxes seem to contain menu items again.  Thanks!!

I did notice, however, that now list boxes contain list items rather than menu items, making them have the same role as the children of a numbered/bulleted list.  Is this intentional?  If not, I'll file a new bug; if so, any chance you could be convinced that the children of combo boxes and list boxes should all be menu items? ;-)

Comment 15

[Aaron Leventhal]

It was intentional, but if GTK does it differently we'll fix it -- but for ATK/AT-SPI only.

One thing that doesn't make ense to me -- is if it's a LISTITEM I would think it would be inside a LIST, and a MENUITEM should be in a MENU.

Comment 16

[Joanmarie Diggs]

> It was intentional, but if GTK does it differently we'll fix it -- but for
> ATK/AT-SPI only.

If it wouldn't be too much trouble, that would be awesome.  Thanks!!
 
> One thing that doesn't make ense to me -- is if it's a LISTITEM I would think
> it would be inside a LIST, and a MENUITEM should be in a MENU.

One would think so, wouldn't one? :-)  I totally see your point.... 

Here's what doesn't make sense to me:  Why are there two very different creatures -- (un)ordered lists, list boxes --  with the same role?  The things one wants to know about one, is very different from the things one wants to know about the other.  Likewise, the things one can do in one is very different from the things one can do in the other.  In my perfect world, we'd have LIST and LISTBOX the children of which would be LISTITEM and LISTBOXITEM respectively. Then there wouldn't be any question about what sort of thing one was in.

Assuming, however, that I must continue to live in an imperfect world..... :-)  Functionally speaking, from the end user's -- and, I'd argue, the AT's -- point of view, a listbox is merely an expanded combo box.* Both exist to allow the user to select the desired option by arrowing to it.  (You, know, like a menu :-) ) Both have very similar states.... 

* With the added possibility of multi-select-ability, of course.

So if fixing this for ATK/AT-SPI is doable, shall I open a new bug for it?

Thanks again!

Comment 17

[Aaron Leventhal]

Spun off bug 380054 for it.