Closed
Bug 584407
Opened 11 years ago
Closed 9 years ago
Create popup menu for inspector HTML panel
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 13
People
(Reporter: rcampbell, Assigned: rcampbell)
References
Details
Attachments
(1 file, 8 obsolete files)
9.51 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•11 years ago
|
Whiteboard: [kd4b5]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rcampbell
Priority: -- → P1
Assignee | ||
Comment 2•11 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•11 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•11 years ago
|
Whiteboard: [kd4b5] → [kd4b6] [strings]
Comment 4•11 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•11 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•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 471826 [details] [diff] [review] HTML Menu WIP Can't see anything bad from here.
Comment 8•11 years ago
|
||
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: ? → ---
Updated•11 years ago
|
Whiteboard: [strings]
Assignee | ||
Comment 10•10 years ago
|
||
clearing priority flag as this feature was dropped from fx4.
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Priority: P1 → --
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•9 years ago
|
||
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)
Comment 13•9 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•9 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•9 years ago
|
||
totally non-qref'd patch. Will update...
Assignee | ||
Comment 16•9 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•9 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•9 years ago
|
||
Updated, still working on the test...
Attachment #592433 -
Attachment is obsolete: true
Attachment #592770 -
Flags: review?(paul)
Assignee | ||
Comment 19•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
menupopup indent fix
Attachment #592779 -
Attachment is obsolete: true
Attachment #592782 -
Flags: review?(paul)
Attachment #592779 -
Flags: review?(paul)
Comment 22•9 years ago
|
||
r=me with the entities in browser.dtd
Assignee | ||
Comment 23•9 years ago
|
||
now with entities in browser.dtd.
Attachment #592782 -
Attachment is obsolete: true
Attachment #592782 -
Flags: review?(paul)
Updated•9 years ago
|
Attachment #592896 -
Flags: review+
Assignee | ||
Comment 24•9 years ago
|
||
Now with Unittest!
Assignee | ||
Updated•9 years ago
|
Whiteboard: [land-in-fx-team]
Comment 25•9 years ago
|
||
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•9 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).
Assignee | ||
Comment 27•9 years ago
|
||
I guess we should remove those empty commandkey entries.
Assignee | ||
Comment 28•9 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•9 years ago
|
||
Attachment #592896 -
Attachment is obsolete: true
Attachment #593189 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
try build here: https://tbpl.mozilla.org/?tree=Try&rev=c8d651ef2357
Updated•9 years ago
|
Whiteboard: [land-in-fx-team]
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/bbffc10af113
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 33•9 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•