User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061213 Minefield/3.0a1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061213 Minefield/3.0a1 Via the AT-SPI, the "Name:" combobox in the "Add Bookmark" dialog seems to show up as a combobox with no children or other needed accessible information, meaning an assistive technology cannot get to what the combobox is displaying. Reproducible: Always Steps to Reproduce: 1. Run Firefox 2. Press Ctrl+D to bring up the "Add Bookmark" dialog 3. Analyze the "Add Bookmark" dialog via at-poke. Actual Results: You'll see that the "Name:" combobox doesn't allow an assistive technology to determine what the combobox is displaying. You'll also see that the "Create in:" combobox in the same dialog does give some good information. Expected Results: We need to be able to get to the accessible text information of the "Name:" field. Maybe it should be an autocomplete widget instead?

confirmed. the "Name" textbox is exposed as combobox because it's <menulist> underneath. see http://landfill.mozilla.org/mxr-test/seamonkey/source/browser/components/bookmarks/content/addBookmark2.xul#91

with dom inspector, menupopup and menuitem under "name" menulist is visible. however, due to no frame, we don't create accessible object for them. at-poke pokes selected children then failed since there is no children. don't know what the solution should be. cc aaron

The bookmark name widget is designed as <menulist> to support microsummary, see http://wiki.mozilla.org/Microsummaries and bug 337825. The problem is, when there is no microsummary, the combobox picker and drop down list is hidden. So only the combobox itself is exposed. However, we didn't use the name of the selected item as the combobox's name, which makes the bookmark name (current selected item) can't be exposed. The way we expose combobox is different from what gtk app does, we can see it in at-poke. To solve this bug, I think we need to make combobox using the selected item's name as its name. This also makes mozilla comply with other gnome apps.

I want the nsIAccessible results to be consistent on Windows, Mac and Linux so we have a consistent scripting environment for extensions like Fire Vox. Also the MSAA name has to remain what it is now. You could change nsComboboxAccessible::GetName() to the ATK behavior but override nsComboBoxAccesibleWrap::get_accName() for MSAA to keep the name it uses now. You probably have to add that class.

should we just override nsComboboxAccessibleWrap::GetName() for ATK or change nsIAccessible result? Aaron, do you need the change for IAccessilbe2 ? And Hakan, could you give some input from Mac's side?

(In reply to comment #5) > should we just override nsComboboxAccessibleWrap::GetName() for ATK or change > nsIAccessible result? I don't know what we're doing in IA2 yet, but I don't want to break JAWS and Window-Eyes now. Ready previous comment again -- the answer is no, don't "override nsComboboxAccessibleWrap::GetName() for ATK". Override get_accName in MSAA instead so you have a consistent result in nsIAccessible::GetName(). I no longer want to override nsIAccessible methods in wrap classes. > And Hakan, could you give some input from Mac's side?

(In reply to comment #3) > The bookmark name widget is designed as <menulist> to support microsummary, see > http://wiki.mozilla.org/Microsummaries and bug 337825. > > The problem is, > when there is no microsummary, the combobox picker and drop down list is > hidden. So only the combobox itself is exposed. However, we didn't use the name > of the selected item as the combobox's name, which makes the bookmark name > (current selected item) can't be exposed. it's not related to the name. it's related to the selection field in at-poke. see my comment #2.

Will, How Orca does with combobox, particularly with combobox's name and selection? As Nian commented, the "Name:" combobox didn't expose the selected item in its "selection", because it's children are invisible. And also, we didn't expose combobox the way what gtk app does, that is, we didn't use the selected item's name as combobox's name. So I want to know how combobox's name and selection will affect Orca. Will it solve your problem that make combobox using the selected item's name as its name? Thanks

(In reply to comment #8) > Will, > How Orca does with combobox, particularly with combobox's name and selection? > > As Nian commented, the "Name:" combobox didn't expose the selected item in its > "selection", because it's children are invisible. And also, we didn't expose > combobox the way what gtk app does, that is, we didn't use the selected item's > name as combobox's name. > > So I want to know how combobox's name and selection will affect Orca. Will it > solve your problem that make combobox using the selected item's name as its > name? Comboboxes are a chore for us to work with in any toolkit, and support tends to differ from toolkit to toolkit. Ideally, the same toolkit would provide consistent behavior for its various uses of comboboxes. In our support for comboboxes provided by Gecko, we currently do the following to find the name of the combobox: 1) First look for something labelling the combobox via the LABEL_FOR relation. 2) Failing #1, we fall back to the name of the combobox as the name of the combobox. Then, we do the following to find what is being presented by the combobox itself: 1) After a discussion I had with Aaron, I thought we concluded that all comboboxes would be guaranteed to have a menu as a child. As such, we look for the menu that's a child of the combobox. 2) If we find a menu, we then look for its accessible selection. If that exists, we say the stuff the combobox is presenting is the name of the first selected object. We usually run into this case. 3) If we find a menu, but no selection, we say the stuff the combobox is presenting is the name of the menu. We also run into this case. 4) If we don't find a menu, we only present what we've guessed as being the label for the object. So...I guess we could always add more logic along the lines of: "well...if there is no menu, we'll use the object name as what is being presented by the combobox, and we'll make sure not to present the name also as the label if they are the same" or something like that. That is, unless you think you'll make this combobox consistent with the rest of the comboboxes used by Gecko. Do you think you'll end up doing that?

Thanks for the detailed explanation! Yes, of course we will keep all comboboxes consistent. The combobox in "Add Bookmark" is more complicated than other, to implement "Microsummary" feature. Currently other combobox can go into case 2) or case 3), but the combobox of bookmark name has no menu at most time. Since "Add Bookmark" is a important function and the combobox problem makes it unusable to disabled users, it's reasonable to make combobox's name as the selected item's name. And also make combobox behavior the same as other gtk applications.

Aaron, I'm not clear about this code in nsAccessible::GetXULName(), http://mxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#1718 To indicate the selected item in accessible's name is just what I want to do to fix this bug. Do you think I can remove "if (!select)" statement? Right now we lost the function of add bookmark to disabled users for most time (unless there really is a microsummary).

> To indicate the selected item in accessible's name is just what I want to do to > fix this bug. Do you think I can remove "if (!select)" statement? Yes, but this does disturb me a bit, because everywhere else in our API support, the name is the associated label. It's true that XUL makes the same mistake that GAIL does, calling the label the currently selected option's text. Here's my summary: 1. We need combo boxes to be spoken correctly by screen readers. 2. Right now Orca is having a problem because Firefox is different from GAIL 3. We would have to write extra code to make Mozilla's combo box name different in ATK vs. MSAA. In ATK it would be the selected item's name instead of the true label, even though in every other control the name is the label. MSAA uses the name for the label. 4. A better option may be to get AT's to use AtkSelection to get the currently selected item. We can stay internally consistent in two ways: form controls would always use the control's true label for the name, and we'd be the same between MSAA and ATK.

I spoke with Peter Parente and Will Walker. Here's the plan: 1. Keep the name as it is 2. Support the selection interface on the ROLE_MENU child (not the combobox as it is now).

(In reply to comment #13) > I spoke with Peter Parente and Will Walker. Here's the plan: > 1. Keep the name as it is > 2. Support the selection interface on the ROLE_MENU child (not the combobox as > it is now). > I agree it's a better solution that to support selection interface, but we have a problem about that. For general combobox, selection interface is already supported. In at-poke we can see the selected option in "selection" list. For the combobox in "Add Bookmark", because the popup menu of the combobox is hidden for most case, we didn't create accessibles for the menu and the options. In at-poke we can see the combobox has no children. I prefer to override GetName for combobox rather than create accessibles for the hidden menu and option.

Let me take a look at it for a while. I am concerned about the long term issue of doing things too differently for different APIs and different controls.

* nsXULMenuPopupAccessible now inherits from nsXULSelectableAccessible * Moved nsXULSelectableAccessible to nsXULMenuAccessible.cpp/.h to avoid a circular dependency in the include files, which would have happened. * While I was there I cleaned up some of the variable names and code in nsXULSelectableAccessible * Keep a convenience variable called mSelectControl which should never be null when selection is a valid thing to deal with. In the nsXULMenuPopupAccessible case, it points the parent node if we're in a combo box, or null otherwise. * Fix nsXULSelectableAccessible::GetSelectedChildren() for single selection case. * HTML combo box no longer supports nsHTMLSelectableAccessible, and thus doesn't support nsIAccessibleSelectable. Other than that, everything was already correct for HTML.

make nsXULMenuPopupAccessible inherits from nsXULSelectableAccessible is much more reasonable. aaron, one question here: did you test it? there is no box model for menu and menuitems in "name" combobox, and hence we won't create an accessible for it. see my comment#2

Yes I tested on Windows and Linux. I did not run into any problem where the option accessibles are not created, and thus the selection interface returns the correct thing. I'm not sure why there would have ever been a problem with that. Maybe someone else should test to make sure I'm not crazy.

Well, the patch I made is useful but doesn't fix the original bug. Now I understand that it's about the fact we don't support these things: <menulist editable="true"> -> entry child needs to be exposed <menulist editable="true" droppable="false"> -> should treat like a textbox <menulist droppable="false"> -> not a big deal, a useless widget

To get the name field to become a combobox, bookmark one of the pages linked to from here: http://wiki.mozilla.org/Microsummaries/Sites That's the only time it really becomes a combobox. Otherwise it's actually just a textbox.

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. one question: why call nsXULMenupopupAccessible::GenerateMenu(mDOMNode) in nsXULButtonAccessible::Init() besides that, r=nian

(In reply to comment #24) > (From update of attachment 253758 [details] [diff] [review]) > one question: why call nsXULMenupopupAccessible::GenerateMenu(mDOMNode) > in > nsXULButtonAccessible::Init() > > besides that, r=nian > If I'm right then GenerateMenu() set @menugenerated attribute on menupopup element. @menugenerated attribute change display of menupopup that leads menuitem elements can have accessible objects.

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. > NS_IMETHODIMP nsXULTextFieldAccessible::Init() > { > CheckForEditor(); >- return nsHyperTextAccessible::Init(); >+ nsresult rv = nsHyperTextAccessible::Init(); >+ nsXULMenupopupAccessible::GenerateMenu(mDOMNode); >+ return NS_OK; > } I guess it is for autocomplete textboxes. Then it won't work since GenerateMenu() won't find <menupopup/> becuase it is anonymous. Btw, why do autocomplete textboxes have accessible for textboxes but not for comboboxes since they are acutally comboboxes (textfield, dropmarker, listbox)?

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. >- return Components.interfaces.nsIAccessibleProvider.XULCombobox; >+ return (this.getAttribute("droppable") == "false") ? >+ Components.interfaces.nsIAccessibleProvider.XULTextBox : >+ Components.interfaces.nsIAccessibleProvider.XULCombobox; I think it's special support for browser/bookmarks? Is there any another places where dropable is used? I think if dropable is not property of toolkit then default menulist accessible shouldn't have its support. Though I am agree it is easiest and probably unique way. At least you should add comment what is dropable and where it is used.

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. >+void nsXULMenupopupAccessible::GenerateMenu(nsIDOMNode *aNode) >+{ I think it would be fine if you add XXX comment because when bug 279703 will be fixed then @menugenerated attribute will gone.

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. >+ >+nsresult nsXULSelectableAccessible::ChangeSelection(PRInt32 aIndex, PRUint8 aMethod, PRBool *aSelState) >+ nsCOMPtr<nsIDOMXULMultiSelectControlElement> xulMultiSelect(do_QueryInterface(mDOMNode)); Should be mSelectControl here since menupopup doesn't implement nsIDOMXULMultiSelectControlElement interface?

> I guess it is for autocomplete textboxes. Then it won't work since > GenerateMenu() won't find <menupopup/> becuase it is anonymous. Okay I'll look at that again. > Btw, why do > autocomplete textboxes have accessible for textboxes but not for comboboxes > since they are acutally comboboxes (textfield, dropmarker, listbox)? We're trying to be consistent with how GTK exposes combos, editable combos and autocompletes. > I think it's special support for browser/bookmarks? Good point, but who knows they may add it to toolkit at some point. Not sure what a better way would be to handle it. I'll add a comment. > I think it would be fine if you add XXX comment because when bug 279703 > will be fixed then @menugenerated attribute will gone. Have you looked at that patch much? Will the frames just be there, and we can get rid of GenerateMenu? Or will GenerateMenu need to change? > Should be mSelectControl here since menupopup doesn't implement > nsIDOMXULMultiSelectControlElement interface? Good catch.

Also, some code styling nits: should be (per http://www.mozilla.org/hacking/mozilla-style-guide.html) if () { } else { } instead of if () { } else { } and if 'if' statement has one expression then braces are not needed like if () // else //

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. >+NS_IMETHODIMP nsXULSelectableAccessible::GetSelectedChildren(nsIArray **aChildren) >+{ >+ *aChildren = nsnull; >+ if (!mSelectControl) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ nsCOMPtr<nsIAccessibilityService> accService = GetAccService(); >+ NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE); >+ >+ nsCOMPtr<nsIMutableArray> selectedAccessibles = >+ do_CreateInstance(NS_ARRAY_CONTRACTID); >+ NS_ENSURE_STATE(selectedAccessibles); >+ >+ // For XUL multi-select control >+ nsCOMPtr<nsIDOMXULMultiSelectControlElement> xulMultiSelect(do_QueryInterface(mDOMNode)); I guess mSelectControl should be here too. Right?

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. Please check the rest methods on mDOMNode using instead of mSelectedNode. >+NS_IMETHODIMP nsXULSelectableAccessible::IsChildSelected(PRInt32 aIndex, PRBool *aIsSelected) >+{ >+ *aIsSelected = PR_FALSE; >+ return ChangeSelection(aIndex, eSelection_GetState, aIsSelected); >+} It looks ChangeSelection() returns NS_ERROR_FAILURE for eSelection_GetState. I think it's wrong. >+ >+NS_IMETHODIMP nsXULSelectableAccessible::ClearSelection() >+{ >+ if (!mSelectControl) { >+ return NS_ERROR_FAILURE; >+ } >+ nsCOMPtr<nsIDOMXULMultiSelectControlElement> xulMultiSelect(do_QueryInterface(mDOMNode)); >+ return xulMultiSelect ? xulMultiSelect->ClearSelection() : mSelectControl->SetSelectedIndex(-1); I think SetSelectedItem(nsnull) should be a bit faster.

Some of the style is inherited from various super reviewers who asked things be a certain way. For example, in nested if's it's bad not to use { and } even for single lines, and that has caused tree breakages before. In addition, I suppose that's the official style guide, but each module sometimes differs. I'm not sure it matters much, but I slightly prefer the way we've been doing things. If you like to do it the official way I won't stop you. I think what matters most is 2 spaces for indent. > I guess mSelectControl should be here too. Right? It's only a multi select when we're in a non-dropdown of some kind. In that case it would be the dom node itself that implements nsIDOMXULMultiSelectControlElement. So I think both cases are right.

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. >+NS_IMETHODIMP nsXULMenuitemAccessible::Init() >+{ >+ nsresult rv = nsAccessibleWrap::Init(); >+ nsXULMenupopupAccessible::GenerateMenu(mDOMNode); >+ return rv; >+} I wonder why this because menuitem hasn't menupopup IIRC? > nsXULMenupopupAccessible::nsXULMenupopupAccessible(nsIDOMNode* aDOMNode, nsIWeakReference* aShell): >- nsAccessibleWrap(aDOMNode, aShell) >+ nsXULSelectableAccessible(aDOMNode, aShell) > { >+ // May be the anonymous <menupopup> inside <menulist> (a combobox) >+ nsCOMPtr<nsIDOMNode> parentNode; >+ aDOMNode->GetParentNode(getter_AddRefs(parentNode)); >+ mSelectControl = do_QueryInterface(parentNode); Please make sure it works for autocomplete textboxes because it shouldn't work now ;) >+ nsresult ChangeSelection(PRInt32 aIndex, PRUint8 aMethod, PRBool *aSelState); >+ nsresult AppendFlatStringFromSubtree(nsIContent *aContent, nsAString *aFlatString) >+ { return NS_ERROR_FAILURE; } // Overrides base impl in nsAccessible Btw, why is NS_ERROR_FAILURE used here?

Also, I wonder why are nsXULTextFieldAccessible changes if autocomplete textbox uses nsXULComboboxAccessible? Is there any usecase when textbox has menupopup?

Comment on attachment 253758 [details] [diff] [review] Previous changes plus: 1) If <menulist droppable="false"> use textbox accessible. 2) Support <menulist editable="true">, 3) Add static method to ::GenerateMenu for menupopups. Canceling review request because I assume there will another patch ;)

(In reply to comment #25) > (In reply to comment #24) > > (From update of attachment 253758 [details] [diff] [review] [details]) > > one question: why call nsXULMenupopupAccessible::GenerateMenu(mDOMNode) > > in > > nsXULButtonAccessible::Init() > > > > besides that, r=nian > > > > If I'm right then GenerateMenu() set @menugenerated attribute on menupopup > element. @menugenerated attribute change display of menupopup that leads > menuitem elements can have accessible objects. > my question is that why call it IN nsXULButtonAccessible::Init()?

> my question is that why call it IN nsXULButtonAccessible::Init()? XUL Buttons like toolbarbutton's can have menupopup children. Actually I'm trying to get this working right now -- my tests are showing that this code isn't doing it's job.

(In reply to comment #39) > > my question is that why call it IN nsXULButtonAccessible::Init()? > XUL Buttons like toolbarbutton's can have menupopup children. Actually I'm > trying to get this working right now -- my tests are showing that this code > isn't doing it's job. > Probably it's because buttons implement neither nsIDOMXULMenuListElement nor nsIDOMXULSelectControlElement interfaces.

Comment on attachment 254183 [details] [diff] [review] Address surkov's comments >? toolkit/content/widgets/q >? accessible/agentvars >? accessible/q >? accessible/build/accessibility.dll.manifest >? accessible/public/msaa/ISimpleDOMText.h >? accessible/public/msaa/ISimpleDOMText_i.c >? accessible/public/msaa/ISimpleDOMText_p.c >? accessible/src/base/nsDocAccessible.new hehe, what's this? :) Looks fine excepting the following issue: >+void nsXULMenupopupAccessible::GenerateMenu(nsIDOMNode *aNode) >+ nsCOMPtr<nsIDOMNodeList> nodes; >+ bindingMgr->GetAnonymousNodesFor(content, getter_AddRefs(nodes)); >+ menuPopup = FindInNodeList(nodeList, nsAccessibilityAtoms::menupopup, >+ kNameSpaceID_XUL); This wont't work because menupopup is not direct anonymous child of textbox at least for ff 2.0. With that r=me.

I'll remove the anon child checks in GenerateMenu

Comment on attachment 254183 [details] [diff] [review] Address surkov's comments Just need r= for toolkit change.

Mano gave me conditional r= over IRC, as long as new accessible object is invalidated if droppable changes.