124452 - Active Accessibility: support HTML's <optgroup> tag

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Aaron Leventhal]

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."

Comment 1

[Aaron Leventhal]

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).

Comment 2

[Peter Trudelle]

nsbeta1+ per ADT triage team

Comment 3

[Bernie McGuire]

Attachment: Initial Patch to create accessibles for <optiongroup> and flatten the tree

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.

Comment 4

[Aaron Leventhal]

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!

Comment 5

[Bernie McGuire]

Attachment: Patch to create accessible for <optgroup>

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

Comment 6

[Bernie McGuire]

Attachment: Patch to create accessible for <optgroup>

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

Comment 7

[Aaron Leventhal]

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.

Comment 8

[Bernie McGuire]

Attachment: Patch rev1 to create accessible for <optgroup>

I beleive this patch addresses all the issues raised above.

Comment 9

[Bernie McGuire]

Attachment: Simple test page with combo box and <option> and <optgroup> test cases

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

Comment 10

[Bernie McGuire]

Attachment: Patch (rev2) to create accessibles for <option> and <optgroup> tags

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.

Comment 11

[Bernie McGuire]

Attachment: Patch (rev3) to create accessibles for <options> and <optgroup>

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

Comment 12

[Aaron Leventhal]

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.

Comment 13

[Bernie McGuire]

Attachment: Changes to patch re comment 12

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

Comment 14

[Bernie McGuire]

Attachment: Simple Test HTML with more <option> and <optgroup>x

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

Comment 15

[Bernie McGuire]

Attachment: Patch (rev4) to create accessibles for <option> and <optgroup>

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.

Comment 16

[Bernie McGuire]

Attachment: Patch (rev5)

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

Comment 17

[Aaron Leventhal]

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

r=aaronl

Comment 18

[Chris Waterson]

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

rs=waterson

Comment 19

[John Gaunt (redfive)]

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

r= jgaunt too, 

Looks good, thanks for the patch Bernie

Comment 20

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 21

[Aaron Leventhal]

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

Comment 22

[Jaime Rodriguez, Jr.]

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

Comment 23

[Aaron Leventhal]

checked into trunk. Thanks Bernie!

Comment 24

[Aaron Leventhal]

oops, really marking fixed.

Comment 25

[Paul Wyskoczka]

Verifying (rs)

Comment 26

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381