567968 - show role and name of event accessible target in event list

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[[e-mail]]

Comment on attachment 447285 [details] [diff] [review]
patch

>+  var role = "";
>+  try {
>+    role = this.mAccService.getStringRole(accessible.role);
>+  }
>+  catch (e) {
>+  }
I don't think this can throw, can it?

>+  var name = "";
>+  try {
>+    name = accessible.name;
>+  }
>+  catch (e) {
>+  }
Is this likely to throw? As far as I can tell it can throw for nodes in deleted documents, but such nodes are unlikely to receive accessible events ;-)

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)

> I don't think this can throw, can it?
> Is this likely to throw? 
> As far as I can tell it can throw for nodes in deleted
> documents, but such nodes are unlikely to receive accessible events ;-)

It shouldn't but sometimes it happens because of wrong a11y invalidation code, so that we still keep an accessible to fire an accessible event but it was shutdown by related invalidation already.

Comment 3

[[e-mail]]

(In reply to comment #2)
> It shouldn't but sometimes it happens because of wrong a11y invalidation code,
> so that we still keep an accessible to fire an accessible event but it was
> shutdown by related invalidation already.
Ah, so if you're worried about the accessible having been shut down then you only need the one try/catch for both statements (since if the first fails then the second would anyway).

I can't apply this to any of my trees, does it depends on another patch?

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Bug 553364 obviates this, right?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #4)
> Bug 553364 obviates this, right?

no, bug 553364 is landed, it's time for review ;)

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 447285 [details] [diff] [review]
patch

Review of attachment 447285 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with one try…catch as in comment 3 and a comment added explaining why.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

try/catch is not needed nowdays (I think starting from Firefox4)

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to alexander surkov from comment #7)
> try/catch is not needed nowdays (I think starting from Firefox4)

(Gecko 2 is not yet the minimum supported version required by DOM Inspector)

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch3

get back try/catch blocks for backward compatibility

Comment 10

[[e-mail]]

Comment on attachment 595653 [details] [diff] [review]
patch3

>+  var id = (node.nodeType == kElementNode) ? node.id : "";
Only HTML and XUL elements have IDs, so you can't use this test.

>+  var role = "";
>+  try {
>+    // may fail prior Gecko 2.0
>+    role = this.mAccService.getStringRole(accessible.role);
>+  } catch(e) {
>+  }
>+  var name = "";
>+  try {
>+    name = accessible.name; // may fail prior Gecko 2.0
>+  } catch(e) {
>+  }
Do you know what causes it to fail prior to Gecko 2.0? Are the causes the same for each block or different?

>+    id: id,
id: node.id || "",
perhaps?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #10)

> Do you know what causes it to fail prior to Gecko 2.0? Are the causes the
> same for each block or different?

I think they should be same, do you want me to put them into single try/catch?

> >+    id: id,
> id: node.id || "",
> perhaps?

yep, I will do this

Comment 12

[[e-mail]]

(In reply to alexander surkov from comment #11)
> (In reply to comment #10)
> > Do you know what causes it to fail prior to Gecko 2.0? Are the causes the
> > same for each block or different?
> I think they should be same, do you want me to put them into single
> try/catch?
Yes please.

Comment 13

[[e-mail]]

Comment on attachment 595653 [details] [diff] [review]
patch3

With changes as discussed.

Comment 14

[alexander :surkov (:asurkov)]

landed with Neil's comments addressed http://hg.mozilla.org/dom-inspector/rev/542c6027c9c0