If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

show role and name of event accessible target in event list

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 447285 [details] [diff] [review]
patch
Attachment #447285 - Flags: superreview?(neil)
Attachment #447285 - Flags: review?(Sevenspade)

Comment 1

7 years ago
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 ;-)
(Assignee)

Comment 2

7 years ago
(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

7 years ago
(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?
Bug 553364 obviates this, right?
(Assignee)

Comment 5

6 years ago
(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 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.
Attachment #447285 - Flags: review?(Sevenspade) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 570456 [details] [diff] [review]
patch2

try/catch is not needed nowdays (I think starting from Firefox4)
Attachment #447285 - Attachment is obsolete: true
Attachment #570456 - Flags: superreview?(neil)
Attachment #447285 - Flags: superreview?(neil)
(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)

Updated

6 years ago
Keywords: access
(Assignee)

Comment 9

6 years ago
Created attachment 595653 [details] [diff] [review]
patch3

get back try/catch blocks for backward compatibility
Attachment #570456 - Attachment is obsolete: true
Attachment #595653 - Flags: superreview?(neil)
Attachment #570456 - Flags: superreview?(neil)
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?
(Assignee)

Comment 11

6 years ago
(In reply to neil@parkwaycc.co.uk 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
(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 on attachment 595653 [details] [diff] [review]
patch3

With changes as discussed.
Attachment #595653 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 14

6 years ago
landed with Neil's comments addressed http://hg.mozilla.org/dom-inspector/rev/542c6027c9c0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 736606
Blocks: 736607
Blocks: 670224
You need to log in before you can comment on or make changes to this bug.