GetName() fails on menupopup

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.83 KB, patch
Aaron Leventhal
: review+
neil@parkwaycc.co.uk
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Created attachment 277544 [details] [diff] [review]
patch
Attachment #277544 - Flags: review?(aaronleventhal)

Comment 1

11 years ago
Comment on attachment 277544 [details] [diff] [review]
patch

> +  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));

I don't think you can do that with a COMPtr. Some compilers can't deal with that.
You can do it with nsIContent* or by using a temp variable and then doing tempParent.swap(content);

Please find out before checking in.
Attachment #277544 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 2

11 years ago
Comment on attachment 277544 [details] [diff] [review]
patch

(In reply to comment #1)
> Please find out before checking in.
> 

It's safe to ask Neil :)
Attachment #277544 - Flags: superreview?(neil)

Comment 3

11 years ago
Comment on attachment 277544 [details] [diff] [review]
patch

>-    nsCOMPtr<nsIDOMNode> parentNode, node(do_QueryInterface(element));
Actually the old code was worse, that node was unnecessary (because you can call GetParentNode on element).

>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  while (content && aName.IsEmpty()) {
>+    content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::label, aName);
>+    content = content->GetParent();
>   }
>+  return NS_OK;
I notice that the old code failed if it found no label, whereas the new code only fails if its node is null. Do you really want this change? If you don't, then you can also remove the null node check, as then you'll fail by not finding a label.
Attachment #277544 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)

> I notice that the old code failed if it found no label, whereas the new code
> only fails if its node is null. Do you really want this change? If you don't,
> then you can also remove the null node check, as then you'll fail by not
> finding a label.
> 

If I got you right then proposed behaviour is bug is about. The method should fail only if mDOMNode is null (that means accessible is out of date). For another cases we shouldn't fail. The main reason is to fail is not good for usage from js.
(Assignee)

Updated

11 years ago
Attachment #277544 - Flags: approval1.9?

Comment 5

11 years ago
(In reply to comment #4)
>If I got you right then proposed behaviour is bug is about.
Ah, I did wonder :-)

Updated

11 years ago
Attachment #277544 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 6

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