nsAccessible::AppendFlatStringFromSubtreeRecurse always returns NS_OK

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 265124 [details] [diff] [review]
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)
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)

Comment 2

12 years ago
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 4

12 years ago
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+
Created attachment 265956 [details]
test case

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

Comment 6

12 years ago
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!

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.