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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

Trunk
x86
Windows XP
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 178609 [details] [diff] [review]
Override GetDescription() for HTML radio buttons in MSAA implementation
(Assignee)

Updated

13 years ago
Attachment #178609 - Flags: review?(timeless)

Updated

13 years ago
Attachment #178609 - Flags: review?(timeless) → review+
(Assignee)

Updated

13 years ago
Attachment #178609 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 2

13 years ago
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)
(Assignee)

Comment 3

13 years ago
(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.
(Assignee)

Updated

13 years ago
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-
(Assignee)

Comment 7

13 years ago
Created attachment 179532 [details] [diff] [review]
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.
Attachment #179532 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #179532 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 8

13 years ago
Created attachment 179541 [details] [diff] [review]
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)
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+
(Assignee)

Comment 10

13 years ago
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
Last Resolved: 13 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.... :(
(Assignee)

Comment 12

13 years ago
Reopening for missing part of checkin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

13 years ago
Created attachment 179910 [details] [diff] [review]
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.
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+
(Assignee)

Updated

13 years ago
Attachment #179910 - Flags: approval1.8b2?

Comment 15

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

a=asa
Attachment #179910 - Flags: approval1.8b2? → approval1.8b2+
(Assignee)

Comment 16

13 years ago
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
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.