287730 - Report correct "n of m" position info for HTML radio buttons

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Spun of from bug 287566

Our algorithm in bug 287566 does not work for HTML radio buttons. The position
of HTML radio buttons and number in each group depends on the name attribute used.

Comment 1

[Aaron Leventhal]

Attachment: Override GetDescription() for HTML radio buttons in MSAA implementation

Comment 2

[[e-mail]]

Comment on attachment 178609 [details] [diff] [review]
Override GetDescription() for HTML radio buttons in MSAA implementation

You should get a content peer to review your changes but I noticed an if
(!radio) { return NS_ERROR_FAILURE; } NS_ASSERTION(radio, ...

Comment 3

[Aaron Leventhal]

(In reply to comment #2)
> (From update of attachment 178609 [details] [diff] [review] [edit])
> You should get a content peer to review your changes but I noticed an if
> (!radio) { return NS_ERROR_FAILURE; } NS_ASSERTION(radio, ...
> 

I'll remove the superfluous assertion.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm pretty swamped with reviews; I may well not get to this for a week or more.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178609 [details] [diff] [review]
Override GetDescription() for HTML radio buttons in MSAA implementation

>Index: content/html/content/public/nsIRadioControlElement.h
>+   * Get the radio group container for this button (form or document)

s/button/radio button/

>Index: content/html/content/public/nsIRadioGroupContainer.h
>+   * @param aPositionIndex out indicates 0-indexed position in the radio group,
>+   *                       but returns -1 if not in group

-1 isn't a valid PRUint32 value... if you mean PR_UINT32_MAX, then say so here,
please....  Alternately, use a PRInt32.

I'm going to need to be a lot more awake to review the rest of this.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178609 [details] [diff] [review]
Override GetDescription() for HTML radio buttons in MSAA implementation

>Index: content/html/content/public/nsIRadioControlElement.h
>+   * Get the radio group container for this button (form or 
document)

s/button/radio button/.  And please don't mention "form or document" -- people
shouldn't care what the container is, exactly, and it may be something else
altogether in the future.

>+  virtual already_AddRefed<nsIRadioGroupContainer> GetRadioGroupContainer() = 0;

Please change nsHTMLInputElement accordingly (move where the method is declared
in the class, fix how it's declared to mention "virtual", and put /* virtual */
before the impl.

>Index: content/html/content/public/nsIRadioGroupContainer.h

>+   * Get the radio group position and the number of radio buttons in the group.

"the" group?  A radio group container can have multiple groups in it.

>+  NS_IMETHOD GetPositionInGroup(nsIDOMHTMLInputElement *aRadio,

What happens if aRadio is not in any of the groups in this container?  Will
this method throw?  Or what?  Your documentation is not consitent with your
implementations....

>Index: content/base/src/nsDocument.cpp

>+  nsAutoString name;
>+  nsCOMPtr<nsIContent> radioContent(do_QueryInterface(aRadio));
>+  NS_ASSERTION(radioContent, "No radio button passed in or no nsIContent for it");

You're passed a nsIDOMHTMLInputElement, which has a GetName method.  Why not
just use that instead of messing with nsIContent and GetAttr?

>Index: content/html/content/src/nsHTMLFormElement.cpp

>+nsHTMLFormElement::GetPositionInGroup(nsIDOMHTMLInputElement 
>+  nsAutoString name;
>+  nsCOMPtr<nsIContent> radioContent(do_QueryInterface(aRadio));
>+  NS_ASSERTION(radioContent, "No radio button passed in or no 
nsIContent for it");

Same here.

>Index: accessible/src/atk/nsHTMLFormControlAccessibleWrap.h

>+typedef class nsHTMLRadioButtonAccessible nsHTMLRadioButtonAccessibleWrap;

>Index: accessible/src/mac/nsHTMLFormControlAccessibleWrap.h

>+typedef class nsHTMLRadioButtonAccessible nsHTMLRadioButtonAccessibleWrap;

>Index: accessible/src/msaa/nsHTMLFormControlAccessibleWrap.cpp

>+nsHTMLRadioButtonAccessibleWrap::nsHTMLRadioButtonAccessibleWrap(nsIDOMNode *aDOMNode, 
>+                                                         nsIWeakReference *aShell):
>+nsHTMLRadioButtonAccessible(aDOMNode, aShell)

So are nsHTMLRadioButtonAccessibleWrap and nsHTMLRadioButtonAccessible the same
class, or different classes?  I'm not sure what those typedefs are aiming to
do... Or is it just that you only need a separate wrap class for msaa?

>nsHTMLRadioButtonAccessibleWrap::GetDescription(nsAString& 
>+      nsTextFormatter::ssprintf(aDescription, L"%d of %d", 

That won't compile on some of our systems, as I'd hope you would know.... If
you need a PRUnichar* for a literal, use NS_LITERAL_STRING("whatever").get().

Is reporting "0 of 0" correct if the radio is not in any group?

Comment 7

[Aaron Leventhal]

Attachment: Address bz's comments

bz, only the MSAA implementation needs to use the wrap class to override the
nsHTMLRadioButtonAccessible behavior.

This is a general thing in the accessibility code. Each platform has the
opportunity to override the XP behavior in nsFooAccessible classes via the
nsFooAccessibleWrap classes. If the platform doesn't need to it just uses a
typedef.

Comment 8

[Aaron Leventhal]

Attachment: Try to address bz's IRC omments

- Added assertions for finding radio group for the specified radio button in
this container.
- Added return value check for GetRadioGroup and ResolveName, to catch any OOM
or other errors
- Not using return value of -1 anymore, the default return value in the worst
case treats us as position 0, numitems 1 (we're a lone wolf radio button)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179541 [details] [diff] [review]
Try to address bz's IRC omments

>Index: content/base/src/nsDocument.cpp
>+  NS_ASSERTION(radioGroup, "No such radio group in this container");
>+  if (!radioGroup) {
>+    return NS_OK;
>+  }

No need for this whole chunk of code, since GetRadioGroup will never return
null together with a success rv.

With that, sr=bzbarsky, though this diff seems to be missing the
accessible/src/msaa/nsHTMLFormControlAccessibleWrap.cpp file; please don't
forget to check that in!

Comment 10

[Aaron Leventhal]

Checking in content/html/content/public/nsIRadioControlElement.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioControlElement.h,v  <-- 
nsIRadioControlElement.h
new revision: 1.4; previous revision: 1.3
done
Checking in content/html/content/public/nsIRadioGroupContainer.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioGroupContainer.h,v  <-- 
nsIRadioGroupContainer.h
new revision: 1.5; previous revision: 1.4
done
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.539; previous revision: 3.538
done
Checking in content/base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.249; previous revision: 3.248
done
Checking in content/html/content/src/nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <-- 
nsHTMLFormElement.cpp
new revision: 1.181; previous revision: 1.180
done
Checking in accessible/src/atk/nsHTMLFormControlAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/atk/nsHTMLFormControlAccessibleWrap.h,v  <-- 
nsHTMLFormControlAccessibleWrap.h
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.134; previous revision: 1.133
done
Checking in accessible/src/mac/Makefile.in;
/cvsroot/mozilla/accessible/src/mac/Makefile.in,v  <--  Makefile.in
new revision: 1.12; previous revision: 1.11
done
Checking in accessible/src/msaa/Makefile.in;
/cvsroot/mozilla/accessible/src/msaa/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <-- 
nsAccessibleWrap.cpp
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/accessible/src/msaa/nsHTMLFormControlAccessibleWrap.cpp,v
done
Checking in accessible/src/msaa/nsHTMLFormControlAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsHTMLFormControlAccessibleWrap.cpp,v  <--
 nsHTMLFormControlAccessibleWrap.cpp
initial revision: 1.1
done
Checking in accessible/src/msaa/nsXULTreeAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsXULTreeAccessibleWrap.cpp,v  <-- 
nsXULTreeAccessibleWrap.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/other/Makefile.in;
/cvsroot/mozilla/accessible/src/other/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er... I just noticed that that last patch (the one that was checked in) doesn't
address one of the review comments from comment 6 (the one starting "Please
change nsHTMLInputElement accordingly").  Could you please fix that?  I guess
you'll need driver approval at this point.... :(

Comment 12

[Aaron Leventhal]

Reopening for missing part of checkin.

Comment 13

[Aaron Leventhal]

Attachment: Change for bz that slipped through the cracks

Sorry about that bz. I had the change but missed getting it uploaded. Thanks
for noticing it was missing.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179910 [details] [diff] [review]
Change for bz that slipped through the cracks

r+sr=bzbarsky, but leave the docs with the method (those document what the
implementation does, which is a litttle more specific than what the interface
documents).

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 179910 [details] [diff] [review]
Change for bz that slipped through the cracks

a=asa

Comment 16

[Aaron Leventhal]

Checking in nsHTMLInputElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v  <-- 
nsHTMLInputElement.cpp
new revision: 1.385; previous revision: 1.384
done

Comment 17

[Firefox Bug Husbandry Bot]

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