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