Closed Bug 584407 Opened 11 years ago Closed 9 years ago

Create popup menu for inspector HTML panel

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

Attachments

(1 file, 8 obsolete files)

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
Duplicate of this bug: 587743
Whiteboard: [kd4b5]
Assignee: nobody → rcampbell
Priority: -- → P1
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.
Status: NEW → ASSIGNED
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.
blocking2.0: --- → ?
Whiteboard: [kd4b5] → [kd4b6] [strings]
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.
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...
Attached patch HTML Menu WIP (obsolete) — Splinter Review
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 on attachment 471826 [details] [diff] [review]
HTML Menu WIP

Can't see anything bad from here.
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: ? → ---
Removing items from kd4b6.
Whiteboard: [kd4b6] [strings] → [strings]
Whiteboard: [strings]
clearing priority flag as this feature was dropped from fx4.
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Priority: P1 → --
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Duplicate of this bug: 705847
Attached patch HTML Menu (obsolete) — Splinter Review
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.
Attachment #471826 - Attachment is obsolete: true
Attachment #592433 - Flags: review?(paul)
Depends on: 719607
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();
Attachment #592433 - Flags: review?(paul) → review-
(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.
totally non-qref'd patch. Will update...
(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.
(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 :)
Attached patch HTML Menu v2 (obsolete) — Splinter Review
Updated, still working on the test...
Attachment #592433 - Attachment is obsolete: true
Attachment #592770 - Flags: review?(paul)
Attached file HTML Menu v2.1 (obsolete) —
let's try that again. This time without that bad hunk.
Attachment #592770 - Attachment is obsolete: true
Attachment #592775 - Flags: review?(paul)
Attachment #592770 - Flags: review?(paul)
Attached patch HTML Menu v2.2 (obsolete) — Splinter Review
added inspector.dtd, fixed indent in jar.mn.
Attachment #592775 - Attachment is obsolete: true
Attachment #592779 - Flags: review?(paul)
Attachment #592775 - Flags: review?(paul)
Attached patch HTML Menu v2.3 (obsolete) — Splinter Review
menupopup indent fix
Attachment #592779 - Attachment is obsolete: true
Attachment #592782 - Flags: review?(paul)
Attachment #592779 - Flags: review?(paul)
r=me with the entities in browser.dtd
Attached patch HTML Menu v3 (obsolete) — Splinter Review
now with entities in browser.dtd.
Attachment #592782 - Attachment is obsolete: true
Attachment #592782 - Flags: review?(paul)
Attachment #592896 - Flags: review+
Attached patch HTML Menu v4 (obsolete) — Splinter Review
Now with Unittest!
Whiteboard: [land-in-fx-team]
Blocks: 724507
Keywords: uiwanted
Whiteboard: [land-in-fx-team]
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  "">

?
(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).
Blocks: 724600
Blocks: 717916
I guess we should remove those empty commandkey entries.
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!
Attachment #593189 - Flags: review?(paul)
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.
Attachment #593189 - Flags: review?(paul) → review+
Attached patch HTML Menu v5Splinter Review
Attachment #592896 - Attachment is obsolete: true
Attachment #593189 - Attachment is obsolete: true
Blocks: 729220
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/bbffc10af113
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bbffc10af113
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.