We need to expose the contents HTML's <optgroup> tag via MSAA. For example: <FORM action="http://somesite.com/prog/someprog" method="post"> <P> <SELECT name="ComOS"> <OPTGROUP label="PortMaster 3"> <OPTION label="3.7.1" value="pm3_3.7.1">PortMaster 3 with ComOS 3.7.1 <OPTION label="3.7" value="pm3_3.7">PortMaster 3 with ComOS 3.7 <OPTION label="3.5" value="pm3_3.5">PortMaster 3 with ComOS 3.5 </OPTGROUP> <OPTGROUP label="PortMaster 2"> <OPTION label="3.7" value="pm2_3.7">PortMaster 2 with ComOS 3.7 <OPTION label="3.5" value="pm2_3.5">PortMaster 2 with ComOS 3.5 </OPTGROUP> <OPTGROUP label="IRX"> <OPTION label="3.7R" value="IRX_3.7R">IRX with ComOS 3.7R <OPTION label="3.5R" value="IRX_3.5R">IRX with ComOS 3.5R </OPTGROUP> </SELECT> </FORM> * The <optgroup> items are not focusable or selectable. * "When specified, user agents should use the value of this [LABEL] attribute rather than the content of the OPTION element as the option label."

As Bernie discovered, the presence of <optgroup> creates a hierarchical structure of DOM nodes. Our combo box accessible code (and probably our listbox code as well), assumes there will only be direct decendant <option>'s. This assumption causes only the <optgroup> nodes to be turned into meaningful accessibles. However, it also creates some trailing garbage accessibles because it still thinks there are more children (although it can't find them).

nsbeta1+ per ADT triage team

This patch will create accessibles for <optiongroup> tags. It will also flatten the tree under a Select List so that both <option> and <optiongroup> tags are at the same level. (This is similar to IE). This flattening has only been tested by looking at the tree in Accessible Explorer. Please test and comment. Additional testing needs to be done using other tools that need accessibles for these tags. Please let me know what you find.

Hi Bernie. Thanks for the patch. I couldn't get it to compile. Here's my error log: nsSelectAccessible.cpp C:\moz\mozilla\accessible\src\base\nsSelectAccessible.cpp(174) : error C2065: 'optionElement' : undeclared identifier C:\moz\mozilla\accessible\src\base\nsSelectAccessible.cpp(183) : error C2501: '_retval' : missing storage-class or type specifiers C:\moz\mozilla\accessible\src\base\nsSelectAccessible.cpp(184) : error C2143: syntax error : missing ';' before 'return' I recommend going to the top of the accessible directory and doing: cvs up Then recompile and put up a new patch. Thanks again!

Sorry for the problem, I did a CVS up and made a new patch. I compiled OK, hope it does the same for you.

Sorry for the problem, I did a CVS up and made a new patch. I compiled OK, hope it does the same for you.

Congrats :) you're close. First, a bunch of style nits: - replace all of your tab with spaces. In Visual Studio's options menu, you should have the option checked to always use spaces, and also select 2 spaces per tab press. I see several places where the curly braces aren't quite right, for example in GetAccessibleFor(). - in your comments in nsAccessibilityService, make sure you name the tag correctly (optgroup not optiongroup). - don't use a lowercase prefix a in variable names, except when a variable is used as an argument to the method you're currently in. - make sure there is always at least 1 blank line between methods, - always use 1 space before an open curly brace at the end of a line - no spaces before semicolons. In nsAccessibilityService::GetAccessibleFor(), the creation of new accessible's can be simplfied: + if (parentAccessible) { + newAcc = new nsHTMLSelectOptGroupAccessible(parentAccessible, aNode, weakShell); + } else { + newAcc = new nsHTMLSelectOptGroupAccessible(nsnull, aNode, weakShell); +} Why not just change that whole thing to the equivalent: + newAcc = new nsHTMLSelectOptGroupAccessible(parentAccessible, aNode, weakShell); Why have that if statement there at all? Also, you have another line to create an accessible if there is no parent node. Rather than have an extra line to do that, you could move the if (parentNode) here: if (parentNode) GetAccessibleFor(parentNode, getter_AddRefs(parentAccessible)); That way, you still have parentAccessible as nsnull, and your newAcc assignment statement still works. Another thing - never use NS_IMETHODIMP in .h files. There you should use NS_IMETHOD. I'm not sure why it compiled. Also, I see you define GetAccNextSibling() in nsSelectAccessible.cpp, but the declaration is commented out in the .h file. I'm not sure why that even compiles. I think nsHTMLSelectOptionAccessible and nsHTMLSelectOptGroupAccessible should be declared/defined next to each other, since one is based off the other, and they are so similar. For nsHTMLSelectOptGroupAccessible::GetAccState(), I wouldn't copy any code. Instead, I would do this: { nsHTMLSelectOptionAccessible::GetAccState(_retval); _retval &= ~STATE_FOCUSABLE|STATE_SELECTABLE; } I would derive nsSelectOptionAccessible from nsLeafAccessible, and I wouldn't override GetAccFirstChild or GetAccLastChild on those. Let the leaf accessible do the work of returning null for those, which is what you want so that option/optgroup accessibles have no children. Right now they're getting children sometimes. nsSelectOptionAccessible::GetAccName needs some work, because on optgroup it is returning a concatentation of all of it's children's names. In addition, tantek's sample page reminds us that if there is a label attribute, it should be used instead of the child text node for the name.

I beleive this patch addresses all the issues raised above.

Simple test HTML with a combo box with different combinations of <option>s and <optgroup> tags. Some with no children, some with no label.

This patch should fix various formatting issues as well as the case where an <optgroup> has no children. Also if an <option> does not have a label the text tag will be used to provide a name.

This patch should make GetAccPrevSibling work with the flattened tree of <option>s and <optgroup>s

More comments: 1. For the following code: // bug 124452 + nsCOMPtr<nsIDOMNode> thisNode(do_QueryInterface(mDOMNode)); + thisNode->GetParentNode(getter_AddRefs(parentNode)); + do { + nsCOMPtr<nsIDOMHTMLSelectElement> selectControl(do_QueryInterface(parentNode)); + if (selectControl) { + break; + } + thisNode = parentNode; + thisNode->GetParentNode(getter_AddRefs(parentNode)); + } while (parentNode); + Why not put this line at the top of the do while loop, so it doesn't need to get repeated: + thisNode->GetParentNode(getter_AddRefs(parentNode)); 2. Also, I think the comment // bug 124452 isn't quite descriptive enough. You could write, // Go up to parent <select> element, or something like that 3. nsHTMLSelectOptGroup::GetAccState(), has incorrect logic It needs to use the value of nsHTMLSelectOption::GetAccState, and clear out the bits STATE_FOCUSABLE and STATE_SELECTABLE. How about this? nsHTMLSelectOption::GetAccState(_retval); *_retval &= ~(STATE_FOCUSABLE|STATE_SELECTABLE); 4. nsHTMLSelectOption::GetAccPreviousSibling() I don't think this will work for the case where you have some options and some optgroups right below the select, which is legal. Since it is rare method to call, why not use the logic I suggested: Just go op to the select list accessible, get the first child, and then keeping getting the next sibling. Stop at the first accessible that is equal to the current one, and return the accessible previous to that.

Re Comment 12: 1. Done, 2. Done, 3. Done, 4. Done.

Test page with additional <option> and <optgroup> tags

This revision to the patch sets mParent of the optionAccessible and optionGroupAccessible correctly to the selectListAccessible in both the Combobox and Listbox. Also GetAccPrevSibling is revised so it can search the Accessible tree for the previous accessible.

Moved the determination of the option(grp) parents from GetAccessibleFor to the constructor for nsSelectOptionAccessible.

Comment on attachment 77267 [details] [diff] [review] Patch (rev5) r=aaronl

Comment on attachment 77267 [details] [diff] [review] Patch (rev5) rs=waterson

Comment on attachment 77267 [details] [diff] [review] Patch (rev5) r= jgaunt too, Looks good, thanks for the patch Bernie

Comment on attachment 77267 [details] [diff] [review] Patch (rev5) a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Still looking for ADT approval. Without this fix, we return garbage options on <selects> that use <optgroup>

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0.

checked into trunk. Thanks Bernie!

oops, really marking fixed.

Verifying (rs)