Louie Zhao Reporter
	Description • 20 years ago

The original design of HTML Combobox can't be recognized by GOK. The new struct
of HTML Combobox is the same as XUL Combobox. Class
nsHTMLComboboxTextFieldAccessible and nsHTMLComboboxButtonAccessible have no use
any more.

Comboboxes:
- nsHTMLComboboxAccessible
  - nsHTMLComboboxListAccessible  [ inserted in accessible tree ]
    - nsHTMLSelectOptionAccessible(s)

I am not sure what purpose the original nsHTMLComboboxAccessible is designed
for. For other apps on Unix/Linux, HTML combobox has the same structure as XUL
combobox (users don't care the difference between XUL and HTML and IMHO
comboboxs should have the same structure).

Verification of this change against MSAA need to be conducted.

	Aaron Leventhal Assignee
	Comment 2 • 20 years ago

There does seem to be a problem with HTML combobox MSAA support today, but is a
different problem. The number of children for the child listbox being reported
is wrong.

However, in MSAA for both XUL and HTML comboboxes, we do the following:

Object with role = combobox
  Child #1 - role = textfield
  Child #2 - role = button
  Child #3 - role = listbox
     Grandchildren = the actual options

I don't see a reason to change anything. Right now, for MSAA, we are exposing
comboboxes the same for XUL and HTML.

	Aaron Leventhal Assignee
	Comment 3 • 20 years ago

Spun off bug 278872 on childcount/children problems for the list box child.

With this bug, are you saying the textfield and drop down button should not be
exposed?

	Aaron Leventhal Assignee
	Comment 4 • 20 years ago

Louie, what do you mean by
>  The new struct of HTML Combobox is the same as XUL Combobox. 

	Louie Zhao Reporter
	Comment 5 • 20 years ago

Gnome applications expose accessible for Combobox as

Object with role = combobox
  Child #1 - role = listbox
     Grandchildren = the actual options

That is to say, there are no "textfield" and "button" child for Combobox. If
"textfield" and "button" are the 1st/2nd child of combox, AT-Tools on Linux
(e.g. GOK) aren't able to access combobox successfully.

Aaron, is there any special reason for combobox accessible to have "textfield"
and "button" as child ? On Linux, we haven't seen the need for that till now.
How about MSAA ? If neccessary, we can limit the change in ATK code.

(In reply to comment #4)
> Louie, what do you mean by
> >  The new struct of HTML Combobox is the same as XUL Combobox. 

In our internal codebase, we have changed XUL Combobox structure first. Sorry
for forgetting that the change hasn't been ported to the trunk.

	Aaron Leventhal Assignee
	Comment 6 • 20 years ago

So far we have emulated what other MSAA applications do, for consistency. I
think that is probably the best choice. So, make the patch ATK-only.

We'll still need the patch from bug 278872. Seeking your r= on that.

	Louie Zhao Reporter
	Comment 7 • 20 years ago

Will update the patch after bug278872 is fixed.

	Philip K. Warren
	Comment 8 • 20 years ago

(In reply to comment #7)
> Will update the patch after bug278872 is fixed.

Bug 278872 is now marked as fixed.

	Louie Zhao Reporter
	Comment 9 • 20 years ago

As discussed before, this patch change the structure of HTML combobox and limit
this change in ATK code. The changes in this patch includes:
1. Make the nsHTMLComboboxListAccessible is the only child of
nsHTMLComboboxAccessibleWrap
2. fix a minor state issue in nsHTMLGroupboxAccessible because it may prevent
other elements (e.g. combobox) to be exposed if they are in "<fieldset>"

	Aaron Leventhal Assignee
	Comment 10 • 19 years ago

Comment on attachment 180012 [details] [diff] [review]
patch v2

First a nit -- there is a spelling error in
mozilla/accessible/src/mac/nsAccessibleWrap.h  (+1 lines) where it says bx
instead of box

Also, the right place to override the construction of children now is in
::CacheChildren() -- we also need to fix the base class to do this
(nsHTMLComboboxAccessible). See some of the other ::CacheChildren() examples --
it sets up mFirstChild and mAccChildCount, and it also sets mNextSibling
correctly on each child. Then you don't need to override GetFirstChild()

	Aaron Leventhal Assignee
	Comment 11 • 19 years ago

Question: in MSAA, the XUL combo box gets exposed like this:

ROLE Combobox
  Child 1 = textfield
  Child 2 = button
  Child 3 = list
     Grandchildren are list items (options)

Isn't that the case in ATK? The textfield and button are anonymous content in
Firefox, and the accessible tree walker automatically creates objects for them.
If ATK isn't exposing the textfield and button, what code is removing them?

	Håkan Waara
	Comment 12 • 18 years ago

Mac also wants comboboxes (and popupmenus & autocomplete too, if possible) to be a single-flattened widget with only a table as child (with rows).

<popupmenu/combobox/autocomplete-textfield>
  <table>
    <item1>
    <item2>
    ...

So I'd be happy if that was implemented in the cross-platform code.

	Håkan Waara
	Comment 13 • 18 years ago

Let me clarify, now that I've looked at popupmenus a bit more. (I'm implementing support for them in bug 362080).

A popupmenu is a button that has as its title the chosen item, and has a ShowMenu action. The menu that pops up is simply a container with a bunch of menuitems...

	Aaron Leventhal Assignee
	Comment 14 • 18 years ago

It looks like we already do XUL combo boxes as this bug suggests we do.

	Aaron Leventhal Assignee
	Comment 15 • 18 years ago

1) Remove unnecessary classes and methods -- however, nsHTMLComboboxTextfieldAccessible and nsHTMLComboboxButtonAccessible are only #ifdef'd for now, and will be completely removed in a followup checkin (where we remove #ifdef COMBO_BOX_WITH_THREE_CHILDREN sections)
2) Create 2 new roles for mapping on each platform -- ROLE_COMBOBOX_LIST and ROLE_COMBOBOX_LISTITEM. Depending on the platform it may be mapped to a menu/menuitem or list/listem (via nsRoleMap.h)
3) For nsHTMLComboboxAccessible, use CacheChildren() to generate children instead. Use the popup list as the only child, in order to make things consistent between platforms.
4) For focused option node, do not return the select itself if nothing is focused, thus avoiding stack overflow loop
5) Fix combo box description to check nsAccessible::GetDescription() first
6) Add default action to both HTML and XUL combo boxes
7) Other minor fixes and cleanups

This also fixes bug 348334 and bug 363849.

It might fix bug 359792 but we need to test.

	Aaron Leventhal Assignee
	Comment 16 • 18 years ago

This patch also fixed a crash I found in the cache which was caused when you focus a combo box and reload. That crash only happened in the debug build and had to do with the text interface test in get_accRole().

	Ginn Chen
	Comment 17 • 18 years ago

Comment on attachment 251029 [details] [diff] [review]
Refactor both HTML and XUL combo boxes

Great patch!

A typo in src/atk/nsRoleMap.h
should be ATK_ROLE_MENU_ITEM

Do we have a plan to remove the COMBO_BOX_WITH_THREE_CHILDREN hunks?

	Nian Liu(n/a in a long time)
	Comment 18 • 18 years ago

one question: user can press 'a' to 'z' to quick access option item. it seems we don't support this.

	alexander :surkov (:asurkov)
	Comment 19 • 18 years ago

(In reply to comment #18)
> one question: user can press 'a' to 'z' to quick access option item. it seems
> we don't support this.
> 

How can FAYT be supported in accessibility?

	alexander :surkov (:asurkov)
	Comment 20 • 18 years ago

(In reply to comment #17)
> 
> Do we have a plan to remove the COMBO_BOX_WITH_THREE_CHILDREN hunks?
> 

I think we can't remove COMBO_BOX_WITH_THREE_CHILDREN since MSAA requires 3 accessible child of combobox.

	Aaron Leventhal Assignee
	Comment 21 • 18 years ago

We can eventually remove COMBO_BOX_WITH_THREE_CHILDREN.

In fact I found no difference with how JAWS and Window-Eyes deal with combo boxes whether the textfield and button are there are not. In fact, that's how our XUL combo boxes were already exposed and they worked fine. So we can be more like ATK/AT-SPI which is very good.

Incremental typing in list boxes doesn't relate to this patch, it's implemented in nsListControlFrame.cpp. Nian, doesn't it work for you?

	alexander :surkov (:asurkov)
	Comment 22 • 18 years ago

(In reply to comment #21)
> We can eventually remove COMBO_BOX_WITH_THREE_CHILDREN.
> 
> In fact I found no difference with how JAWS and Window-Eyes deal with combo
> boxes whether the textfield and button are there are not. In fact, that's how
> our XUL combo boxes were already exposed and they worked fine. So we can be
> more like ATK/AT-SPI which is very good.

But this may broke other screen readers for Windows. I guess then we can add Wrap classes in MSAA for comboboxes and override CacheChildren() method there.

	Aaron Leventhal Assignee
	Comment 23 • 18 years ago

Alexander, no other screen readers for Windows currently support Firefox. Second, the events on the combobox and list items are still the same and provide all the info the screen readers need. They're not looking at those other 2 objects because they don't need them. Finally, look at the fact that our XUL combo boxes have already been like this, so any screen readers would have had to support that already.

In any case, I'm leaving the #ifdef there so that we can easily undo that if we need to. If we do actually decide to keep things as they were. It's no problem to override in the Wrap classes, although I wouldn't do it in anything that affects nsIAccessible. The new goal is to put the changes only in the final mapping layer so that scripting with nsIAccessible stays consistent. It's possible to strip out objects at the last minute but keep them in the nsIAccessible hierarchy. See MustPrune() for example.

	alexander :surkov (:asurkov)
	Comment 24 • 18 years ago

Comment on attachment 251029 [details] [diff] [review]
Refactor both HTML and XUL combo boxes


>   *     - nsHTMLSelectListAccessible
>   *       - nsHTMLSelectOptionAccessible
>   *
>   *  Comboboxes:
>   *     - nsHTMLComboboxAccessible
>-  *        - nsHTMLComboboxTextFieldAccessible
>-  *        - nsHTMLComboboxButtonAccessible
>   *        - nsHTMLComboboxListAccessible  [ inserted in accessible tree ]
>   *           - nsHTMLSelectOptionAccessible(s)
>   */

I think you shouldn't remove these lines. Here it's fine to mention COMBO_BOX_WITH_THREE_CHILDREN.

> NS_IMETHODIMP nsHTMLSelectListAccessible::GetRole(PRUint32 *_retval)
> {
>-  *_retval = ROLE_LIST;
>+  *_retval = ROLE_COMBOBOX_LIST;

> NS_IMETHODIMP nsHTMLSelectOptionAccessible::GetRole(PRUint32 *aRole)
> {
>-  *aRole = ROLE_LISTITEM;
>+  *aRole = ROLE_COMBOBOX_LISTITEM;

Why? I guess roles should depend on this is for listbox or combobox.

>+void nsHTMLComboboxAccessible::CacheChildren()
> {
>-  nsHTMLSelectableAccessible::Shutdown();
>+  if (!mWeakShell) {
>+    // This node has been shut down
>+    mAccChildCount = eChildCountUninitialized;
>+    return;
>+  }
> 
>-  mComboboxTextFieldAccessible = nsnull;
>-  mComboboxButtonAccessible = nsnull;
>-  mComboboxListAccessible = nsnull;
>+  if (mAccChildCount == eChildCountUninitialized) {
>+    mAccChildCount = 0;
>+#ifdef COMBO_BOX_WITH_THREE_CHILDREN
>+    // We no longer create textfield and button accessible, in order to have
>+    // harmonization between IAccessible2, ATK/AT-SPI and OS X

nit: this comment should be rather positive than negative statement

>+    nsHTMLComboboxTextFieldAccessible* textFieldAccessible = 
>+      new nsHTMLComboboxTextFieldAccessible(this, mDOMNode, mWeakShell);

I don't think mDOMNode is right to use here.

>+    nsHTMLComboboxButtonAccessible* buttonAccessible =
>+      new nsHTMLComboboxButtonAccessible(mParent, mDOMNode, mWeakShell);

Here too. Right?

>   *  Listbox:
>   *     - nsXULListboxAccessible
>-  *        - nsXULSelectListAccessible
>-  *           - nsXULSelectOptionAccessible
>+  *        - nsXULSelectOptionAccessible

nsXULSelectOptionAccessible is removed

>   *  Comboboxes:
>   *     - nsXULComboboxAccessible      <menulist />
>-  *        - nsHTMLTextFieldAccessible
>-  *        - nsXULComboboxButtonAccessible    
>-  *        - nsXULSelectListAccessible      <menupopup />
>-  *           - nsXULSelectOptionAccessible(s)   <menuitem />
>+  *        - nsXULMenuAccessible          <menupopup />
>+  *           - nsXULMenuitemAccessible(s)   <menuitem />
>   */

Also it's better to don't remove this, just comment it.

>   *        - nsXULSelectListAccessible
>   *           - nsXULSelectOptionAccessible

Please check this comment too.

>   *  Comboboxes:
>   *     - nsXULComboboxAccessible
>-  *        - nsHTMLTextFieldAccessible (editable) or nsTextAccessible (readonly)
>-  *        - nsXULComboboxButtonAccessible
>-  *         - nsXULSelectListAccessible
>-  *            - nsXULSelectOptionAccessible
>+  *        - nsXULMenuAccessible
>+  *          - nsXULSelectOptionAccessible

this too.

> 
> NS_IMETHODIMP nsXULListboxAccessible::GetRole(PRUint32 *_retval)
> {
>-  *_retval = ROLE_LIST;
>+  *_retval = ROLE_COMBOBOX_LIST;
>   return NS_OK;

Why?

> NS_IMETHODIMP nsXULListitemAccessible::GetState(PRUint32 *_retval)
> {
>   if (mIsCheckbox) {
>     nsXULMenuitemAccessible::GetState(_retval);
>+    *_retval |= STATE_CHECKABLE;
>     return NS_OK;

What about STATE_CHECKED?

	Aaron Leventhal Assignee
	Comment 25 • 18 years ago

Comments addressed. You caught some mistakes where I should have used the COMBOBOX roles.

Regarding your last comment, about STATE_CHECKED in nsXULListitemAccessible::GetState(), it turns out I don't need to do checkable either. It's handled by the upcall to sXULMenuitemAccessible::GetState()

The mDOMNode usage for those anonymous accessibles in HTML combo boxes are correct, because they don't have their own DOM nodes -- just their own frames. If we start using the code again we should probably pass the frame in as we're doing for the nsHTMLComboboxListAccessible.

	alexander :surkov (:asurkov)
	Comment 27 • 18 years ago

(In reply to comment #25)

> The mDOMNode usage for those anonymous accessibles in HTML combo boxes are
> correct, because they don't have their own DOM nodes -- just their own frames.
> If we start using the code again we should probably pass the frame in as we're
> doing for the nsHTMLComboboxListAccessible.
> 

Ah, I slept yesterday :)

Also one nit:

  *  Listbox:
  *     - nsXULListboxAccessible
  *        - nsXULSelectListAccessible
  *        - nsXULSelectOptionAccessible

You removed nsXULSelectOptionAccessible and nsXULSelectListAccessible, I guess it should be

  *  Listbox:
  *     - nsXULListboxAccessible
  *        -  nsXULListitemAccessible

	Aaron Leventhal Assignee
	Comment 28 • 18 years ago

r=me on that comment change and I'll check it in :)