584407 - Create popup menu for inspector HTML panel

Status: RESOLVED FIXED

Description

[Rob Campbell [:rc] (:robcee)]

There are likely a few operations we'll want to perform on HTML nodes. We should expose these in a popup menu.

Undo
--------
Cut Node
Copy Node
Paste Node
--------
Revert Changes
--------
Scroll into view
--------
Find

Comment 2

[Rob Campbell [:rc] (:robcee)]

each one of these features is going to be a feature of its own. We'll need to at least stub this menu to get strings. Taking this for b5.

Comment 3

[Kevin Dangoor]

Rob has a mostly-there patch for this now. If this is going to make it, we'll need to try to get it in beta 6 because of the strings.

Comment 4

[Axel Hecht [:Pike]]

Is this still in for B6? If so, can we at least see a WIP patch here? Looks like this is hard to even triage in this state.

Comment 5

[Rob Campbell [:rc] (:robcee)]

I'm scaling back on some of the features in the menu. I think we can safely go for:

Edit
--------
Cut
Copy
--------
Scroll into view

for starters.

WIP patch attaching...

Comment 6

[Rob Campbell [:rc] (:robcee)]

Created attachment 471826 [details] [diff] [review]
HTML Menu WIP

very WIP, bitrotted. Just putting here to get some l10n eyeballs on it. Hoping to get back to this patch this weekend to finish it up.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 471826 [details] [diff] [review]
HTML Menu WIP

Can't see anything bad from here.

Comment 8

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 9

[Kevin Dangoor]

Removing items from kd4b6.

Comment 10

[Rob Campbell [:rc] (:robcee)]

clearing priority flag as this feature was dropped from fx4.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Created attachment 592433 [details] [diff] [review]
HTML Menu

here's a version that successfully deletes what it's supposed to. Adding a test now.

One thing I'm wondering if invalidateHierarchy() in breadcrumbs is the best way to deal with this. Seems to work fine, but I tried using cutAfter without success. Let me know what you think.

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 592433 [details] [diff] [review]
HTML Menu

Looks good. r- for the moment, but mostly nits.

>+++ b/browser/base/content/browser-sets.inc
>@@ -139,13 +139,6 @@
>     <command id="Browser:ToggleAddonBar" oncommand="toggleAddonBar();"/>
>   </commandset>
> 
>-  <commandset id="placesCommands">
>-    <command id="Browser:ShowAllBookmarks"
>-             oncommand="PlacesCommandHook.showPlacesOrganizer('AllBookmarks');"/>
>-    <command id="Browser:ShowAllHistory"
>-             oncommand="PlacesCommandHook.showPlacesOrganizer('History');"/>
>-  </commandset>
>-

Why?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -242,6 +242,22 @@
>       </hbox>
>     </panel>
> 
>+    <menupopup id="inspectorHTMLPopup">

Can you rename it in a way that it can be used by the infobar? (bug 717919)
This might be extended later for pseudo-classes/elements, so it's not HTML-specific.

id="inspectorNodeMenu" ?

>+      <menuitem id="inspectorHTMLCopyInner"
>+                label="&inspectorHTMLCopyInner.label;"
>+                accesskey="&inspectorHTMLCopyInner.accesskey;"
>+                command="Inspector:CopyInner"/>
>+      <menuitem id="inspectorHTMLCopyOuter"
>+                label="&inspectorHTMLCopyOuter.label;"
>+                accesskey="&inspectorHTMLCopyOuter.accesskey;"
>+                command="Inspector:CopyOuter"/>
>+      <menuseparator/>
>+      <menuitem id="inspectorHTMLDelete"
>+                label="&inspectorHTMLDelete.label;"
>+                accesskey="&inspectorHTMLDelete.accesskey;"
>+                command="Inspector:DeleteNode"/>
>+  </menupopup>
>+

indentation

>diff --git a/browser/devtools/highlighter/TreePanel.jsm b/browser/devtools/highlighter/TreePanel.jsm
>+  deleteChildBox: function TP_deleteChildNode(aElement)
>+  {
>+    this.IUI._log("deleting");
>+
>+    let childBox = this.ioBox.findObjectBox(aElement);
>+    if (!childBox) {
>+      this.IUI._log("no childBox");
>+      return;
>+    }
>+    let parentBox = childBox.parentNode;
>+    parentBox.removeChild(childBox);
>+  },
>+

Can we get a comment for this function?

>diff --git a/browser/devtools/highlighter/inspector.jsm b/browser/devtools/highlighter/inspector.jsm
>+  /**
>+   * Copy the innerHTML of the selected Node to the clipboard. Called via the
>+   * HTML panel's context menu.
>+   */

>+  /**
>+   * Copy the outerHTML of the selected Node to the clipboard. Called via the
>+   * HTML panel's context menu.
>+   */

>+  /**
>+   * Delete the selected node. Called via the HTML panel's context menu.
>+   */

If we agree that this menu can be reused somewhere else (infobar), remove "Called via the HTML panel's context menu".

>+  deleteNode: function IUI_deleteNode()
>+  {
>+    let selection = this.selection;
>+    let parent = this.selection.parentNode;
>+
>+    // select the parent node in the highlighter, treepanel, breadcrumbs
>+    this.inspectNode(parent);
>+
>+    // remove the node from the treepanel
>+    this.treePanel.deleteChildBox(selection, parent);
>+
>+    // remove the node from content
>+    parent.removeChild(selection);
>+
>+    // XXX todo remove the node from breadcrumbs

I think this should work:
this.breadcrumbs.invalidateHierarchy();
this.breadcrumbs.udpate();

Comment 14

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (robcee) from comment #12)
> One thing I'm wondering if invalidateHierarchy() in breadcrumbs is the best
> way to deal with this. Seems to work fine, but I tried using cutAfter
> without success. Let me know what you think.

Don't use cutAfter. This will make things a little more complicated, it's not worth it.

Comment 15

[Rob Campbell [:rc] (:robcee)]

totally non-qref'd patch. Will update...

Comment 16

[Rob Campbell [:rc] (:robcee)]

(In reply to Paul Rouget [:paul] from comment #13)
> Comment on attachment 592433 [details] [diff] [review]
> HTML Menu
> 
> Looks good. r- for the moment, but mostly nits.
> 
> >+++ b/browser/base/content/browser-sets.inc
> >@@ -139,13 +139,6 @@
> >     <command id="Browser:ToggleAddonBar" oncommand="toggleAddonBar();"/>
> >   </commandset>
> > 
> >-  <commandset id="placesCommands">
> >-    <command id="Browser:ShowAllBookmarks"
> >-             oncommand="PlacesCommandHook.showPlacesOrganizer('AllBookmarks');"/>
> >-    <command id="Browser:ShowAllHistory"
> >-             oncommand="PlacesCommandHook.showPlacesOrganizer('History');"/>
> >-  </commandset>
> >-
> 
> Why?

This was not the right patch. I posted a previous version. Sorry about that.

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> >--- a/browser/base/content/browser.xul
> >+++ b/browser/base/content/browser.xul
> >@@ -242,6 +242,22 @@
> >       </hbox>
> >     </panel>
> > 
> >+    <menupopup id="inspectorHTMLPopup">
> 
> Can you rename it in a way that it can be used by the infobar? (bug 717919)
> This might be extended later for pseudo-classes/elements, so it's not
> HTML-specific.
> 
> id="inspectorNodeMenu" ?

how about "inspector-node-popup"?

> >+      <menuitem id="inspectorHTMLCopyInner"
> >+                label="&inspectorHTMLCopyInner.label;"
> >+                accesskey="&inspectorHTMLCopyInner.accesskey;"
> >+                command="Inspector:CopyInner"/>
> >+      <menuitem id="inspectorHTMLCopyOuter"
> >+                label="&inspectorHTMLCopyOuter.label;"
> >+                accesskey="&inspectorHTMLCopyOuter.accesskey;"
> >+                command="Inspector:CopyOuter"/>
> >+      <menuseparator/>
> >+      <menuitem id="inspectorHTMLDelete"
> >+                label="&inspectorHTMLDelete.label;"
> >+                accesskey="&inspectorHTMLDelete.accesskey;"
> >+                command="Inspector:DeleteNode"/>
> >+  </menupopup>
> >+
> 
> indentation

there is nothing wrong with that indentation. :)

> 
> >diff --git a/browser/devtools/highlighter/TreePanel.jsm b/browser/devtools/highlighter/TreePanel.jsm
> >+  deleteChildBox: function TP_deleteChildNode(aElement)
> >+  {
> >+    this.IUI._log("deleting");
> >+
> >+    let childBox = this.ioBox.findObjectBox(aElement);
> >+    if (!childBox) {
> >+      this.IUI._log("no childBox");
> >+      return;
> >+    }
> >+    let parentBox = childBox.parentNode;
> >+    parentBox.removeChild(childBox);
> >+  },
> >+
> 
> Can we get a comment for this function?

yes, they're commented in the real version. Honest! :(

> >diff --git a/browser/devtools/highlighter/inspector.jsm b/browser/devtools/highlighter/inspector.jsm
> >+  /**
> >+   * Copy the innerHTML of the selected Node to the clipboard. Called via the
> >+   * HTML panel's context menu.
> >+   */
> 
> >+  /**
> >+   * Copy the outerHTML of the selected Node to the clipboard. Called via the
> >+   * HTML panel's context menu.
> >+   */
> 
> >+  /**
> >+   * Delete the selected node. Called via the HTML panel's context menu.
> >+   */
> 
> If we agree that this menu can be reused somewhere else (infobar), remove
> "Called via the HTML panel's context menu".

Ah, good. And yes, there's no reason we can't reuse it for... stuff.

> >+  deleteNode: function IUI_deleteNode()
> >+  {
> >+    let selection = this.selection;
> >+    let parent = this.selection.parentNode;
> >+
> >+    // select the parent node in the highlighter, treepanel, breadcrumbs
> >+    this.inspectNode(parent);
> >+
> >+    // remove the node from the treepanel
> >+    this.treePanel.deleteChildBox(selection, parent);
> >+
> >+    // remove the node from content
> >+    parent.removeChild(selection);
> >+
> >+    // XXX todo remove the node from breadcrumbs
> 
> I think this should work:
> this.breadcrumbs.invalidateHierarchy();
> this.breadcrumbs.udpate();

What I ended up doing was reordering the sequence here. By calling invalidateHierarchy then inspectNode, all seems to work and should cut down on duplication of breadcrumbs calling.

Comment 17

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (robcee) from comment #16)
> > Can you rename it in a way that it can be used by the infobar? (bug 717919)
> > This might be extended later for pseudo-classes/elements, so it's not
> > HTML-specific.
> > 
> > id="inspectorNodeMenu" ?
> 
> how about "inspector-node-popup"?

sure.

> > indentation
> 
> there is nothing wrong with that indentation. :)

My bad :)

Comment 18

[Rob Campbell [:rc] (:robcee)]

Created attachment 592770 [details] [diff] [review]
HTML Menu v2

Updated, still working on the test...

Comment 19

[Rob Campbell [:rc] (:robcee)]

Created attachment 592775 [details]
HTML Menu v2.1

let's try that again. This time without that bad hunk.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Created attachment 592779 [details] [diff] [review]
HTML Menu v2.2

added inspector.dtd, fixed indent in jar.mn.

Comment 21

[Rob Campbell [:rc] (:robcee)]

Created attachment 592782 [details] [diff] [review]
HTML Menu v2.3

menupopup indent fix

Comment 22

[Paul Rouget [:paul]]

r=me with the entities in browser.dtd

Comment 23

[Rob Campbell [:rc] (:robcee)]

Created attachment 592896 [details] [diff] [review]
HTML Menu v3

now with entities in browser.dtd.

Comment 24

[Rob Campbell [:rc] (:robcee)]

Created attachment 593189 [details] [diff] [review]
HTML Menu v4

Now with Unittest!

Comment 25

[Dão Gottwald [::dao]]

Comment on attachment 593189 [details] [diff] [review]
HTML Menu v4

>+    <menupopup id="inspector-node-popup">
>+      <menuitem id="inspectorHTMLCopyInner"
>+                label="&inspectorHTMLCopyInner.label;"
>+                accesskey="&inspectorHTMLCopyInner.accesskey;"
>+                command="Inspector:CopyInner"/>
>+      <menuitem id="inspectorHTMLCopyOuter"
>+                label="&inspectorHTMLCopyOuter.label;"
>+                accesskey="&inspectorHTMLCopyOuter.accesskey;"
>+                command="Inspector:CopyOuter"/>
>+      <menuseparator/>
>+      <menuitem id="inspectorHTMLDelete"
>+                label="&inspectorHTMLDelete.label;"
>+                accesskey="&inspectorHTMLDelete.accesskey;"
>+                command="Inspector:DeleteNode"/>
>+    </menupopup>

Why is this in browser.xul rather than inspector.html?

>+<!ENTITY inspectorHTMLCopyInner.commandkey  "">

?

Comment 26

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #25)
> Comment on attachment 593189 [details] [diff] [review]
> HTML Menu v4
> 
> >+    <menupopup id="inspector-node-popup">
> >+      <menuitem id="inspectorHTMLCopyInner"
> >+                label="&inspectorHTMLCopyInner.label;"
> >+                accesskey="&inspectorHTMLCopyInner.accesskey;"
> >+                command="Inspector:CopyInner"/>
> >+      <menuitem id="inspectorHTMLCopyOuter"
> >+                label="&inspectorHTMLCopyOuter.label;"
> >+                accesskey="&inspectorHTMLCopyOuter.accesskey;"
> >+                command="Inspector:CopyOuter"/>
> >+      <menuseparator/>
> >+      <menuitem id="inspectorHTMLDelete"
> >+                label="&inspectorHTMLDelete.label;"
> >+                accesskey="&inspectorHTMLDelete.accesskey;"
> >+                command="Inspector:DeleteNode"/>
> >+    </menupopup>
> 
> Why is this in browser.xul rather than inspector.html?

We will need this menu in other parts of the inspector: infobar (bug 717916) and breadcrumbs (bug 724600).

Comment 27

[Rob Campbell [:rc] (:robcee)]

I guess we should remove those empty commandkey entries.

Comment 28

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 593189 [details] [diff] [review]
HTML Menu v4

+<!ENTITY inspectorHTMLCopyInner.label       "Copy Inner HTML">
+<!ENTITY inspectorHTMLCopyInner.accesskey   "I">
+<!ENTITY inspectorHTMLCopyInner.commandkey  "">

I'm going to remove the commandkey strings.

Paul, can you give the browser.xul, browser-sets.inc and browser.dtd bits a quick once-over (with the above removal in mind)? Thanks!

Comment 29

[Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)]

Comment on attachment 593189 [details] [diff] [review]
HTML Menu v4

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

::: browser/base/content/browser-sets.inc
@@ +156,5 @@
>      <command id="Inspector:HTMLPanel"
>               oncommand="InspectorUI.toggleHTMLPanel();"/>
> +    <command id="Inspector:CopyInner" oncommand="InspectorUI.copyInnerHTML();"/>
> +    <command id="Inspector:CopyOuter" oncommand="InspectorUI.copyOuterHTML();"/>
> +    <command id="Inspector:DeleteNode" oncommand="InspectorUI.deleteNode();"/>

Nit: the rest of these split the oncommand on to a new line, so for consistency's sake, let's do that here.

::: browser/base/content/browser.xul
@@ +242,4 @@
>        </hbox>
>      </panel>
>  
> +    <menupopup id="inspector-node-popup">

This id stood out as "wrong" to me but there isn't a great deal of consistency within browser.xul about ids for context menus, so follow your heart here.

Comment 30

[Rob Campbell [:rc] (:robcee)]

Created attachment 599079 [details] [diff] [review]
HTML Menu v5

Comment 31

[Rob Campbell [:rc] (:robcee)]

try build here: https://tbpl.mozilla.org/?tree=Try&rev=c8d651ef2357

Comment 32

[Paul Rouget [:paul]]

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

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

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