385024 - Fieldset html tag creates 'panel' accessible with wrong children

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

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.

Comment 1

[Scott Haeger]

Attachment: minimal test case

Comment 2

[Aaron Leventhal]

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?

Comment 3

[Scott Haeger]

I am not concerned with the ROLE_PANEL.  It seems correct.  I am more concerned about the <dead> children accessibles.

Comment 4

[Aaron Leventhal]

Attachment: Multiple fixes, see comment

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.

Comment 5

[Aaron Leventhal]

Attachment: Also fix relations in XUL groupboxes and remove extra GetState() impl there

Comment 6

[alexander :surkov (:asurkov)]

  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());

Comment 7

[Aaron Leventhal]

> 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?

Comment 8

[Aaron Leventhal]

CC'ing Surkov since he wasn't CC'd for comment 7.

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[alexander :surkov (:asurkov)]

(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 :)

Comment 11

[Aaron Leventhal]

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

Comment 12

[Aaron Leventhal]

Attachment: Address surkov's comments

Comment 13

[Aaron Leventhal]

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

Comment 14

[alexander :surkov (:asurkov)]

Attachment: 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

Comment 15

[alexander :surkov (:asurkov)]

(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<>?

Comment 16

[Aaron Leventhal]

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

Comment 17

[Aaron Leventhal]

Attachment: Allow the legend to not be the first child, and only accept the first legend as a label for the fieldset

Comment 18

[Aaron Leventhal]

Attachment: Fieldset with 2 legend elements as children, neither as the first child

Comment 19

[alexander :surkov (:asurkov)]

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.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: groupbox testcase

Comment 21

[Aaron Leventhal]

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

Comment 22

[alexander :surkov (:asurkov)]

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

Comment 23

[Aaron Leventhal]

Attachment: Address surkov's comments

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 270306 [details] [diff] [review]
Address surkov's comments

r=me