Create popup menu for inspector HTML panel

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

unspecified
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

7 years ago
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
(Assignee)

Updated

7 years ago
Duplicate of this bug: 587743

Updated

7 years ago
Whiteboard: [kd4b5]
(Assignee)

Updated

7 years ago
Assignee: nobody → rcampbell
Priority: -- → P1
(Assignee)

Comment 2

7 years ago
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

Comment 3

7 years ago
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: --- → ?

Updated

7 years ago
Whiteboard: [kd4b5] → [kd4b6] [strings]

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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...
(Assignee)

Comment 6

7 years ago
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

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

Can't see anything bad from here.

Comment 8

7 years ago
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: ? → ---

Comment 9

7 years ago
Removing items from kd4b6.
Whiteboard: [kd4b6] [strings] → [strings]
Whiteboard: [strings]
(Assignee)

Comment 10

7 years ago
clearing priority flag as this feature was dropped from fx4.
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Priority: P1 → --
(Assignee)

Updated

6 years ago
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 705847
(Assignee)

Comment 12

5 years ago
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.
Attachment #471826 - Attachment is obsolete: true
Attachment #592433 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Depends on: 719607

Comment 13

5 years ago
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-

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
totally non-qref'd patch. Will update...
(Assignee)

Comment 16

5 years ago
(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

5 years ago
(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 :)
(Assignee)

Comment 18

5 years ago
Created attachment 592770 [details] [diff] [review]
HTML Menu v2

Updated, still working on the test...
Attachment #592433 - Attachment is obsolete: true
Attachment #592770 - Flags: review?(paul)
(Assignee)

Comment 19

5 years ago
Created attachment 592775 [details]
HTML Menu v2.1

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)
(Assignee)

Comment 20

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

added inspector.dtd, fixed indent in jar.mn.
Attachment #592775 - Attachment is obsolete: true
Attachment #592779 - Flags: review?(paul)
Attachment #592775 - Flags: review?(paul)
(Assignee)

Comment 21

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

menupopup indent fix
Attachment #592779 - Attachment is obsolete: true
Attachment #592782 - Flags: review?(paul)
Attachment #592779 - Flags: review?(paul)

Comment 22

5 years ago
r=me with the entities in browser.dtd
(Assignee)

Comment 23

5 years ago
Created attachment 592896 [details] [diff] [review]
HTML Menu v3

now with entities in browser.dtd.
Attachment #592782 - Attachment is obsolete: true
Attachment #592782 - Flags: review?(paul)

Updated

5 years ago
Attachment #592896 - Flags: review+
(Assignee)

Comment 24

5 years ago
Created attachment 593189 [details] [diff] [review]
HTML Menu v4

Now with Unittest!
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]

Updated

5 years ago
Blocks: 724507

Updated

5 years ago
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  "">

?

Comment 26

5 years ago
(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).

Updated

5 years ago
Blocks: 724600

Updated

5 years ago
Blocks: 717916
(Assignee)

Comment 27

5 years ago
I guess we should remove those empty commandkey entries.
(Assignee)

Comment 28

5 years ago
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+
(Assignee)

Comment 30

5 years ago
Created attachment 599079 [details] [diff] [review]
HTML Menu v5
Attachment #592896 - Attachment is obsolete: true
Attachment #593189 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
try build here: https://tbpl.mozilla.org/?tree=Try&rev=c8d651ef2357
(Assignee)

Updated

5 years ago
Blocks: 729220

Updated

5 years ago
Whiteboard: [land-in-fx-team]

Comment 32

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.