Rob Campbell [:rc] (:robcee) Reporter
	Description • 12 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

	Paul Rouget [:paul]
	Comment 1 • 12 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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 3 • 12 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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 4 • 12 years ago

(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.

	Joe Walker [:jwalker]
	Comment 5 • 12 years ago

Comment on attachment 718453 [details] [diff] [review]
Patch v2

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

Looks good to me.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 6 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/b7f366cbd4e2

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/b7f366cbd4e2