Closed Bug 287730 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(2 files, 2 obsolete files)

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.
Attachment #178609 - Flags: review?(timeless)
Attachment #178609 - Flags: review?(timeless) → review+
Attachment #178609 - Flags: superreview?(neil.parkwaycc.co.uk)
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, ...
Attachment #178609 - Flags: superreview?(neil.parkwaycc.co.uk)
(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.
Attachment #178609 - Flags: superreview?(bzbarsky)
I'm pretty swamped with reviews; I may well not get to this for a week or more.
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 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?
Attachment #178609 - Flags: superreview?(bzbarsky) → superreview-
Attached patch Address bz's comments (obsolete) — Splinter Review
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.
Attachment #179532 - Flags: superreview?(bzbarsky)
Attachment #179532 - Flags: superreview?(bzbarsky)
- 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)
Attachment #178609 - Attachment is obsolete: true
Attachment #179532 - Attachment is obsolete: true
Attachment #179541 - Flags: superreview?(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!
Attachment #179541 - Flags: superreview?(bzbarsky) → superreview+
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.... :(
Reopening for missing part of checkin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry about that bz. I had the change but missed getting it uploaded. Thanks
for noticing it was missing.
Attachment #179910 - Flags: superreview?(bzbarsky)
Attachment #179910 - Flags: review?(timeless)
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).
Attachment #179910 - Flags: superreview?(bzbarsky)
Attachment #179910 - Flags: superreview+
Attachment #179910 - Flags: review?(timeless)
Attachment #179910 - Flags: review+
Attachment #179910 - Flags: approval1.8b2?
Comment on attachment 179910 [details] [diff] [review]
Change for bz that slipped through the cracks

a=asa
Attachment #179910 - Flags: approval1.8b2? → approval1.8b2+
Checking in nsHTMLInputElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v  <-- 
nsHTMLInputElement.cpp
new revision: 1.385; previous revision: 1.384
done
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.