Last Comment Bug 584407 - Create popup menu for inspector HTML panel
: Create popup menu for inspector HTML panel
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 13
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
: 587743 705847 (view as bug list)
Depends on: 572038 719607
Blocks: 717916 724507 724600 729220
  Show dependency treegraph
 
Reported: 2010-08-04 09:18 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2012-02-27 01:19 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
HTML Menu WIP (8.47 KB, patch)
2010-09-03 06:43 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
HTML Menu (6.30 KB, patch)
2012-01-28 12:46 PST, Rob Campbell [:rc] (:robcee)
paul: review-
Details | Diff | Splinter Review
HTML Menu v2 (6.35 KB, patch)
2012-01-30 10:31 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
HTML Menu v2.1 (5.80 KB, text/plain)
2012-01-30 10:37 PST, Rob Campbell [:rc] (:robcee)
no flags Details
HTML Menu v2.2 (7.07 KB, patch)
2012-01-30 10:45 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
HTML Menu v2.3 (7.06 KB, patch)
2012-01-30 10:52 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
HTML Menu v3 (5.59 KB, patch)
2012-01-30 15:38 PST, Rob Campbell [:rc] (:robcee)
paul: review+
Details | Diff | Splinter Review
HTML Menu v4 (9.60 KB, patch)
2012-01-31 12:44 PST, Rob Campbell [:rc] (:robcee)
paul: review+
Details | Diff | Splinter Review
HTML Menu v5 (9.51 KB, patch)
2012-02-21 03:02 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description User image Rob Campbell [:rc] (:robcee) 2010-08-04 09:18:46 PDT
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 1 User image Rob Campbell [:rc] (:robcee) 2010-08-17 06:06:50 PDT
*** Bug 587743 has been marked as a duplicate of this bug. ***
Comment 2 User image Rob Campbell [:rc] (:robcee) 2010-08-20 08:55:21 PDT
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 User image Kevin Dangoor 2010-08-26 09:44:25 PDT
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 User image Axel Hecht [:Pike] 2010-09-01 14:16:38 PDT
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 User image Rob Campbell [:rc] (:robcee) 2010-09-03 06:41:14 PDT
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 User image Rob Campbell [:rc] (:robcee) 2010-09-03 06:43:14 PDT
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 User image Axel Hecht [:Pike] 2010-09-03 06:49:36 PDT
Comment on attachment 471826 [details] [diff] [review]
HTML Menu WIP

Can't see anything bad from here.
Comment 8 User image Kevin Dangoor 2010-09-03 19:56:02 PDT
Inspector feature postponed. Removing blocking flags from Inspector bugs.
Comment 9 User image Kevin Dangoor 2010-09-03 20:38:17 PDT
Removing items from kd4b6.
Comment 10 User image Rob Campbell [:rc] (:robcee) 2010-11-22 07:14:45 PST
clearing priority flag as this feature was dropped from fx4.
Comment 11 User image Rob Campbell [:rc] (:robcee) 2012-01-25 08:47:26 PST
*** Bug 705847 has been marked as a duplicate of this bug. ***
Comment 12 User image Rob Campbell [:rc] (:robcee) 2012-01-28 12:46:44 PST
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 User image Paul Rouget [:paul] 2012-01-29 03:13:46 PST
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 User image Paul Rouget [:paul] 2012-01-29 03:21:47 PST
(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 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:08:10 PST
totally non-qref'd patch. Will update...
Comment 16 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:21:44 PST
(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 User image Paul Rouget [:paul] 2012-01-30 10:29:14 PST
(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 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:31:37 PST
Created attachment 592770 [details] [diff] [review]
HTML Menu v2

Updated, still working on the test...
Comment 19 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:37:57 PST
Created attachment 592775 [details]
HTML Menu v2.1

let's try that again. This time without that bad hunk.
Comment 20 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:45:04 PST
Created attachment 592779 [details] [diff] [review]
HTML Menu v2.2

added inspector.dtd, fixed indent in jar.mn.
Comment 21 User image Rob Campbell [:rc] (:robcee) 2012-01-30 10:52:19 PST
Created attachment 592782 [details] [diff] [review]
HTML Menu v2.3

menupopup indent fix
Comment 22 User image Paul Rouget [:paul] 2012-01-30 11:17:59 PST
r=me with the entities in browser.dtd
Comment 23 User image Rob Campbell [:rc] (:robcee) 2012-01-30 15:38:15 PST
Created attachment 592896 [details] [diff] [review]
HTML Menu v3

now with entities in browser.dtd.
Comment 24 User image Rob Campbell [:rc] (:robcee) 2012-01-31 12:44:06 PST
Created attachment 593189 [details] [diff] [review]
HTML Menu v4

Now with Unittest!
Comment 25 User image Dão Gottwald [:dao] 2012-02-06 05:25:26 PST
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 User image Paul Rouget [:paul] 2012-02-06 10:33:02 PST
(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 User image Rob Campbell [:rc] (:robcee) 2012-02-09 07:22:52 PST
I guess we should remove those empty commandkey entries.
Comment 28 User image Rob Campbell [:rc] (:robcee) 2012-02-09 13:09:22 PST
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 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-13 15:48:55 PST
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 User image Rob Campbell [:rc] (:robcee) 2012-02-21 03:02:30 PST
Created attachment 599079 [details] [diff] [review]
HTML Menu v5
Comment 31 User image Rob Campbell [:rc] (:robcee) 2012-02-21 03:05:39 PST
try build here: https://tbpl.mozilla.org/?tree=Try&rev=c8d651ef2357
Comment 33 User image Tim Taubert [:ttaubert] 2012-02-27 01:19:27 PST
https://hg.mozilla.org/mozilla-central/rev/bbffc10af113

Note You need to log in before you can comment on or make changes to this bug.