Closed Bug 385024 Opened 16 years ago Closed 16 years ago

Fieldset html tag creates 'panel' accessible with wrong children

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: scott, Assigned: aaronlev)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(5 files, 4 obsolete files)

The attached fieldset.html demonstrates the problem.  The fieldset html tag is creating a 'panel' accessible with four children instead of the expected two.  Children 2 and 4 are <dead> accessibles as seen by Accerciser.
Attached file minimal test case
Are you reporting a problem with the children only? Or are you also saying that it should have a different role than ROLE_PANEL? If so, what role should it have?
I am not concerned with the ROLE_PANEL.  It seems correct.  I am more concerned about the <dead> children accessibles.
Attached patch Multiple fixes, see comment (obsolete) — Splinter Review
1) Make fieldset a hypertextaccesible
2) Remove special CacheChildren() that hides legend
3) Fix legend so is related via "label for" to fieldset
4) Remove GetState() impl so it can have states like OFFSCREEN/SHOWING/VISIBLE/OPAQUE etc. We don't need special GetState() impls any longer -- we used to in order to clear STATE_FOCUSABLE but now nsAccessible::GetState() is smart about FOCUSABLE.

This is much better, because a legend can have non-text children like an image or even an input box. We should not have been trying to remove it from the hierarchy.
Attachment #269369 - Flags: review?(ginn.chen)
Attachment #269369 - Attachment is obsolete: true
Attachment #269380 - Flags: review?(ginn.chen)
Attachment #269369 - Flags: review?(ginn.chen)
  Lines 2720-2732    NS_IMETHODIMP nsAccessible::GetAccessibl

+ accService->GetAccessibleInWeakShell(relatedNode, mWeakShell, aRelated);

Why don't you check nsresult anymore?

nsIDOMNode* nsHTMLGroupboxAccessible::GetLegend()

It would be safely to use already_addRefed here because it seems you will loose memory on

nsCOMPtr<nsIContent> legendContent = do_QueryInterface(GetLegend());
> Why don't you check nsresult anymore?
We've been using nsresult incorrectly in the nsAccessibilityService::GetAccessible*() methods. This method shouldn't throw an exception when no accessible is returned -- it's a normal situation.

> nsIDOMNode* nsHTMLGroupboxAccessible::GetLegend()
> It would be safely to use already_addRefed here because it seems you will loose
> memory on
> nsCOMPtr<nsIContent> legendContent = do_QueryInterface(GetLegend());
Are you sure? Well, I don't mind adding already_AddRefed.

Do you need a new patch for already_AddRefed or can I get r+ if I promise to make that change?
CC'ing Surkov since he wasn't CC'd for comment 7.
+ aName.Truncate();  // Default name is blank 

nit: I guess we don't need the comment because we should clear the value in anyway.

fieldset gets first legend in tree but legend checks only parent. You may get that fieldset is labeled by legend by legend hasn't that fieldset as a parent.

I assume xul:caption has label role, correct? Also why do you check only first child if it is of label role? For example, if it is parsed xul then there may be whitespaces and you don't find label.
(In reply to comment #7)
> > Why don't you check nsresult anymore?
> We've been using nsresult incorrectly in the
> nsAccessibilityService::GetAccessible*() methods. This method shouldn't throw
> an exception when no accessible is returned -- it's a normal situation.

Do we have bug to fix nsresult using in GetAccessible*() methods?

> > nsIDOMNode* nsHTMLGroupboxAccessible::GetLegend()
> > It would be safely to use already_addRefed here because it seems you will loose
> > memory on
> > nsCOMPtr<nsIContent> legendContent = do_QueryInterface(GetLegend());
> Are you sure? Well, I don't mind adding already_AddRefed.

No, it looks there is no memory leaks. But already_AddRefed looks usually here :)

> Do you need a new patch for already_AddRefed or can I get r+ if I promise to
> make that change?
> 

No, with answered/fixed comments from previous comment :)
> + aName.Truncate();  // Default name is blank 
Removed in next patch

> fieldset gets first legend in tree but legend checks only parent. You may get
> that fieldset is labeled by legend by legend hasn't that fieldset as a parent.
Fixed in next patch, which requires a parent/firstchild relationship

> I assume xul:caption has label role, correct? 
Yes.

> Also why do you check only first
> child if it is of label role? For example, if it is parsed xul then there may
> be whitespaces and you don't find label.
Actually we don't create accessible object for the whitespaces.

> Do we have bug to fix nsresult using in GetAccessible*() methods?
No, I'll file it

> No, it looks there is no memory leaks. But already_AddRefed looks usually here
:)
Boris Zbarsky told me we use already_AddRefed<> too much, and should only use it when it's really needed. It generates extra code for an extra addref/release.
Attached patch Address surkov's comments (obsolete) — Splinter Review
Attachment #269380 - Attachment is obsolete: true
Attachment #269648 - Flags: review?(surkov.alexander)
Attachment #269380 - Flags: review?(ginn.chen)
> Do we have bug to fix nsresult using in GetAccessible*() methods?
It was bug 347019 but it doesn't like like that fix was complete.

We still return error when accessible is not created, even if there simply is no need to create an accessible for that DOM node.
Attached file fieldset test
>+  nsCOMPtr<nsIAccessible> testLabelAccessible;
>+  GetFirstChild(getter_AddRefs(testLabelAccessible));
>+  if (testLabelAccessible && Role(testLabelAccessible) == nsIAccessibleRole::ROLE_LABEL) {
>+    nsCOMPtr<nsIAccessNode> labelAccessNode(do_QueryInterface(testLabelAccessible));
>+    NS_ENSURE_TRUE(labelAccessNode, nsnull);
>+    nsCOMPtr<nsIDOMNode> labelNode;
>+    labelAccessNode->GetDOMNode(getter_AddRefs(labelNode));
>+    nsCOMPtr<nsIContent> labelContent = do_QueryInterface(labelNode);
>+    if (labelContent && labelContent->NodeInfo()->Equals(nsAccessibilityAtoms::legend,
>+        kNameSpaceID_None)) {
>+      return labelNode;
>+    }
>+  }

legend may not be first child
(In reply to comment #11)

> Boris Zbarsky told me we use already_AddRefed<> too much, and should only use
> it when it's really needed. It generates extra code for an extra
> addref/release.
> 

What the rule is when I should use already_AddRefed<>?
> legend may not be first child
Damn it, you're right.

> What the rule is when I should use already_AddRefed<>?
If you know it's safe to remove it, then don't use it. But, you can ask him on IRC for more detail if you want. It might be a good thing for Mozilla to write up in the XPCOM FAQ's on developer.mozilla.org somewhere.
Attachment #269648 - Attachment is obsolete: true
Attachment #269733 - Flags: review?(surkov.alexander)
Attachment #269648 - Flags: review?(surkov.alexander)
Comment on attachment 269733 [details] [diff] [review]
Allow the legend to not be the first child, and only accept the first legend as a label for the fieldset


>+  nsCOMPtr<nsIDOMNode> legendNode;
>+  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
>+  if (element) {

Should we fail here instead?

>+    nsCOMPtr<nsIDOMNodeList> legends;
>+    nsAutoString nameSpaceURI;
>+    element->GetNamespaceURI(nameSpaceURI);
>+    element->GetElementsByTagNameNS(nameSpaceURI, NS_LITERAL_STRING("legend"),
>+                                  getter_AddRefs(legends));
>+    if (legends) {

IIRC if the method doesn't fail then legends is not nsnull.

>+
>+NS_IMETHODIMP
>+nsHTMLGroupboxAccessible::GetAccessibleRelated(PRUint32 aRelationType,
>+                                               nsIAccessible **aRelated)
>+{

Won't you use NS_ENSURE_ARG_POINTER here?

>+  if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
>+    // Look for groupbox parent
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(mDOMNode);
>+    if (!content) {
>+      return NS_ERROR_FAILURE;  // Node already shut down
>     }
>+    nsCOMPtr<nsIAccessible> groupboxAccessible = GetParent();

You check here parent only though GetElementsByTagName() traverses all tree.

>-NS_IMETHODIMP
>-nsXULGroupboxAccessible::GetState(PRUint32 *aState, PRUint32 *aExtraState)
>+NS_IMETHODIMP nsXULGroupboxAccessible::GetName(nsAString& aName)

there are advantages in syntax
NS_IMETHODIMP
nsClass::
than
NS_IMETHODIMP nsClass::

These are: lines are shorter (arguments are fitted into 80 charachters restriction) and patch doesn't show NS_IMETHODIMP which makes it more readable. I would vote for the first way.

> {
>-  // Groupbox doesn't support focusable state!
>-  nsresult rv = nsAccessible::GetState(aState, aExtraState);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  aName.Truncate();  // Default name is blank 

nit: you wanted to remove a comment?

>+  if (aRelationType == nsIAccessibleRelation::RELATION_LABELLED_BY) {
>+    // The accessible <label> for <groupbox> is an anonymous child of 
>+    // the <caption>

It sounds like label for xul:groupbox is generated from xul:label that is inside of anonymous content of xul:caption and xul:caption is not accessible. Can you clear the comment?

>+    nsCOMPtr<nsIAccessible> testLabelAccessible;
>+    GetFirstChild(getter_AddRefs(testLabelAccessible));

xul:caption can be any child of xul:groupbox and moreover there may be several xul:caption that are used to describe xul:groupbox. I'll attach a testcase.
> Should we fail here instead?
As far as AT is concerned, I don't think it's an error if there is no legend. They just would expect a blank accessible name.
(In reply to comment #21)
> > Should we fail here instead?
> As far as AT is concerned, I don't think it's an error if there is no legend.
> They just would expect a blank accessible name.
> 

Ok, though it means something wrong with accessible object.
Attachment #269733 - Attachment is obsolete: true
Attachment #270306 - Flags: review?(surkov.alexander)
Attachment #269733 - Flags: review?(surkov.alexander)
Comment on attachment 270306 [details] [diff] [review]
Address surkov's comments

r=me
Attachment #270306 - Flags: review?(surkov.alexander) → review+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.