Closed
Bug 567968
Opened 14 years ago
Closed 13 years ago
show role and name of event accessible target in event list
Categories
(Other Applications :: DOM Inspector, defect)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: access)
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
neil
:
superreview+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #447285 -
Flags: superreview?(neil)
Attachment #447285 -
Flags: review?(Sevenspade)
|
||
Comment 1•14 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•14 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•14 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?
|
||
Comment 4•13 years ago
|
||
Bug 553364 obviates this, right?
|
|
Assignee | |
Comment 5•13 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 6•13 years ago
|
||
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•13 years ago
|
||
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)
|
|
||
Comment 8•13 years ago
|
||
(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)
|
|
Assignee | |
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
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•13 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
|
|
||
Comment 12•13 years ago
|
||
(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•13 years ago
|
||
Comment on attachment 595653 [details] [diff] [review]
patch3
With changes as discussed.
Attachment #595653 -
Flags: superreview?(neil) → superreview+
|
|
Assignee | |
Comment 14•13 years ago
|
||
landed with Neil's comments addressed http://hg.mozilla.org/dom-inspector/rev/542c6027c9c0
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
|
|
||
Updated•13 years ago
|
Blocks: DOMi2.0.11
You need to log in
before you can comment on or make changes to this bug.
Description
•