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

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.

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.

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

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.

Removing items from kd4b6.

clearing priority flag as this feature was dropped from fx4.

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

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

Updated, still working on the test...

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

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

menupopup indent fix

r=me with the entities in browser.dtd

now with entities in browser.dtd.

Now with Unittest!

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).

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!

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.

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