381048 - nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: possible fix

This can be problematic. For example, in nsAccessible::GetHTMLName, the last case code that looks for the title attribute value is never hit (as AppendFlatStringFromSubtree always succeeds).

We could either have nsAccessible::AppendFlatStringFromSubtreeRecurse not always return NS_OK, or perhaps AppendFlatStringFromSubtree could return an NS_ERROR_FAILURE if the out string variable is still empty? (I'll throw up a patch illustrating the latter approach)

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: maybe this is better

Comment 2

[Aaron Leventhal]

What bug does this actually fix? What if something is supposed to be empty?

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

In GetHTMLName:

  if (aCanAggregateSubtree) {
    // Don't use AppendFlatStringFromSubtree for container widgets like menulist
    nsresult rv = AppendFlatStringFromSubtree(content, &aLabel);
    if (NS_SUCCEEDED(rv) && !aLabel.IsEmpty()) {
      return NS_OK;
    }
  }

  // Still try the title as as fallback method in that case.
  if (!content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::title,
                        aLabel)) {
    aLabel.SetIsVoid(PR_TRUE);
  }
  return NS_OK;


Without the && !aLabel.IsEmpty() check... the subsequent code to use title value is never hit AFAICT.  AppendFlatStringFromSubtree always succeeds.

This was discovered when considering ways to design/implement dojo menuitem in HTML. We found another way, but this still seems like an undesirable bug.

In GOK we did a lot of this kind of thing... if we got a name that was empty, we kept trying other routes to get a more informative name (so we could make a gok key with a label).

But perhaps I'm missing something in the code path here?

Comment 4

[Aaron Leventhal]

Comment on attachment 265126 [details] [diff] [review]
maybe this is better

r+ if you attach a minimal HTML testcase to the bug we can be clear about the before and after.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: test case

Aaron, here's a test case; sorry for the delay.

Comment 6

[Aaron Leventhal]

Thanks. Can you ask Gijs or someone to check it in for you?

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Yes. Today was busy busy... I'll pop on the channel at some point tomorrow I hope!