Closed Bug 381048 Opened 16 years ago Closed 16 years ago

nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: davidb, Assigned: davidb)

Details

Attachments

(3 files)

Attached patch possible fixSplinter Review
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)
Summary: nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK → title attribute missed because nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK
Summary: title attribute missed because nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK → nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK
Assignee: aaronleventhal → david.bolter
Status: NEW → ASSIGNED
Attachment #265126 - Flags: review?(aaronleventhal)
What bug does this actually fix? What if something is supposed to be empty?
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 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.
Attachment #265126 - Flags: review?(aaronleventhal) → review+
Attached file test case
Aaron, here's a test case; sorry for the delay.
Thanks. Can you ask Gijs or someone to check it in for you?
Yes. Today was busy busy... I'll pop on the channel at some point tomorrow I hope!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.