[inspector] Right click on a doctype node in the markup panel throws errors and causes weird things to happen

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rc, Assigned: miker)

Tracking

unspecified
Firefox 22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
STR:

Open the Inspector, open the Markup panel.
Right click on a doctype and select Delete Node from the context menu.

Weird things happen, including two errors on the Error Console. E.g.,

Timestamp: 2012-09-24 3:54:40 PM
Error: TypeError: this.node.classList is undefined
Source File: resource:///modules/highlighter.jsm
Line: 611

Comment 1

6 years ago
After inspector refactoring, I now see:

Error: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [inIDOMUtils.hasPseudoClassLock]
Source File: resource://gre/modules/devtools/InspectorPanel.jsm
Line: 342
Priority: -- → P2
Summary: Right click on a doctype node in the markup panel throws errors and causes weird things to happen → [inspector] Right click on a doctype node in the markup panel throws errors and causes weird things to happen

Updated

5 years ago
Blocks: 831711
OS: Mac OS X → All
QA Contact: mratcliffe
Hardware: x86 → All
Created attachment 716510 [details] [diff] [review]
Patch

This patch adds a little context menu intelligence:
1. Pseudo classes can only be set on element nodes as they are the only nodes that contain styles.
2. Deleting doctype nodes is forbidden as the spec does not allow them to be edited.
3. Copy unique selector is not valid for doctype nodes as they cannot be accessed using selectors.
Attachment #716510 - Flags: review?(fayearthur)
Whiteboard: [has-patch]
Assignee: nobody → mratcliffe

Comment 3

5 years ago
Comment on attachment 716510 [details] [diff] [review]
Patch

>+    // Disable "Copy Unique Selector" if needed
>+    let unique = this.panelDoc.getElementById("node-menu-copyuniqueselector");
>+    if (this.selection.isDocumentTypeNode()) {
>+      unique.setAttribute("disabled", "true");
>+    } else {
>+      unique.removeAttribute("disabled");
>+    }

Shouldn't we enable "copy unique selector" only if this.select.isElementNode()?

This needs tests.
Attachment #716510 - Flags: review?(fayearthur) → review-
Created attachment 718453 [details] [diff] [review]
Patch v2

(In reply to Paul Rouget [:paul] from comment #3)
> Comment on attachment 716510 [details] [diff] [review]
> Patch
> 
> >+    // Disable "Copy Unique Selector" if needed
> >+    let unique = this.panelDoc.getElementById("node-menu-copyuniqueselector");
> >+    if (this.selection.isDocumentTypeNode()) {
> >+      unique.setAttribute("disabled", "true");
> >+    } else {
> >+      unique.removeAttribute("disabled");
> >+    }
> 
> Shouldn't we enable "copy unique selector" only if
> this.select.isElementNode()?
> 

Yes, we should also only show "copy inner/outerhtml" entries for elements. Both fixed.

> This needs tests.

Done.
Attachment #716510 - Attachment is obsolete: true
Attachment #718453 - Flags: review?(paul)

Updated

5 years ago
Attachment #718453 - Flags: review?(paul) → review?(jwalker)
Comment on attachment 718453 [details] [diff] [review]
Patch v2

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

Looks good to me.
Attachment #718453 - Flags: review?(jwalker) → review+
Whiteboard: [has-patch] → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/b7f366cbd4e2
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b7f366cbd4e2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22

Updated

5 years ago
Depends on: 847314
No longer depends on: 847314
You need to log in before you can comment on or make changes to this bug.