Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Create popup menu for inspector HTML panel

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
RESOLVED FIXED
7 years ago
6 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

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

Comment 12

6 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

6 years ago
Depends on: 719607

Comment 13

6 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

6 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

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

Comment 16

6 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

6 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

6 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

6 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

6 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

6 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

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

Comment 23

6 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

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

Comment 24

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

Now with Unittest!
(Assignee)

Updated

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

Updated

6 years ago
Blocks: 724507

Updated

6 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

6 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

6 years ago
Blocks: 724600

Updated

6 years ago
Blocks: 717916
(Assignee)

Comment 27

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

Comment 28

6 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

6 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

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

Updated

6 years ago
Blocks: 729220

Updated

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

Comment 32

6 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: 6 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.