Closed
Bug 993416
Opened 11 years ago
Closed 11 years ago
[markup view] Context menu to paste HTML in a node
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: pbro, Assigned: gormanchi)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [mentor=bgrins][good first bug][lang=js])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
We already have the "edit as HTML" context menu in the markup view. It basically turns the node into an HTML editor, lets you edit it as you want, and submit the changes.
The goal here is to add a new context menu option that lets you paste HTML code you might have in your clipboard directly in the node.
Firebug has a similar feature and let's you choose to insert the HTML in the following ways:
- replace the outerHTML (the node itself)
- replace the innerHTML (the content only)
- append as a firstchild or last child
- append before or after (as a sibling)
Reporter | ||
Comment 1•11 years ago
|
||
Given what we have in place, this looks like a good-first-bug
Priority: -- → P2
Whiteboard: [good first bug][lang=js]
Comment 2•11 years ago
|
||
I would remove "append as a firstchild or last child" since that doesn't map really well into the existing actor method for setOuterHTML: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#1727. In fact, we don't have a method for setInnerHTML either (though we should). It may be best to split that out into another bug since it involves modifying actor methods.
To simplify this bug further - let's just add a single context menu item (keeping it as a slide out in order to make it easy to add more in the future):
- Paste HTML
-- Paste outerHTML
Whiteboard: [good first bug][lang=js] → [mentor=bgrins][good first bug][lang=js]
Reporter | ||
Updated•11 years ago
|
Blocks: firebug-gaps
I would like to work on this please.
Can you point me to where the "Edit as HTML" functionality is implemented? I think it will be a good place for me to start getting familiar with how the code works.
Thanks!
Actually, I managed to figure out where the XUL and string resources for the inspector are, so I think I can make progress on this. If I have any questions, I'll post further. Please assign the bug to me. :)
After making changes to the XUL, JS, and string resources for the inspector, what is the quickest way to see them in my build? I'm currently doing a mach build from the root, but was wondering if there was something faster.
Proposed changes without tests. Tests are in progress.
Attachment #8405964 -
Flags: review?(bgrinstead)
I tried adding the item to a slide-out menu as suggested, but it turns out the context menu being used in the inspector doesn't support slide-outs currently. Not sure if that's why the copy menu items are organized the way they are.
I am adding tests for these changes to browser_inspector_menu.js. Stay tuned for the next review. I just wanted to get some early feedback in case I'm doing something wrong.
Reporter | ||
Comment 8•11 years ago
|
||
(In reply to Gorman Ho from comment #5)
> After making changes to the XUL, JS, and string resources for the inspector,
> what is the quickest way to see them in my build? I'm currently doing a mach
> build from the root, but was wondering if there was something faster.
`mach build` supports incremental builds (see https://wiki.mozilla.org/DevTools/Hacking#Incremental_Builds).
What I end up doing most of the time is `mach build browser/devtools toolkit/devtools` just to rebuild the ui toolbox part and the server-side code, if I made any changes to it.
In your case, you could do `mach build browser/devtools` since most of the files you changed are in this directory (one note though: you changed a l10n dtd file, which is in browser, so at least one `mach build browser` would have been needed).
Reporter | ||
Comment 9•11 years ago
|
||
Great work! I'll let Brian comment on the patch, but this is definitely going in the right direction as far as I can see.
I've assigned the bug to you.
Thanks a lot!
Assignee: nobody → gormanchi
Assignee | ||
Comment 10•11 years ago
|
||
Thanks for assigning the bug and for the advice. One more question: how do I run the tests in browser_inspector_menu.js?
Reporter | ||
Comment 11•11 years ago
|
||
Tests are run with the `mach mochitest-browser` command: https://wiki.mozilla.org/DevTools/Hacking#Browser_Mochitests
If that's not working for you right now (with a fresh update of fx-team), that's because I think there may be a problem with the command right now.
We're migrating to a new command: `mach mochitest-devtools` but it seems like it's not accepting test folder parameters yet.
I'll report back when I know more.
Reporter | ||
Comment 12•11 years ago
|
||
For info, you can follow the bug about the mach test command here: bug 995972
Comment 13•11 years ago
|
||
Comment on attachment 8405964 [details] [diff] [review]
993416.patch
Review of attachment 8405964 [details] [diff] [review]:
-----------------------------------------------------------------
This is looking great so far! I've made a couple of comments.
Can you also please add a new test named browser_markupview_html_paste.js or similar for this in browser/markupview/test? You will also need to add this test filename to the browser.ini in that same directory. This test will not need to be as extensive as the browser_markupview_html_edit_* tests, since they already cover a bunch of the edge cases with editing outerHTML. We will just want to cover the basic functionality of inspector.pasteOuterHTML - making sure it ignores images, empty clipboard text, etc. You can set contents on the clipboard for the test using require("sdk/clipboard").set(data, datatype).
::: browser/devtools/inspector/inspector-panel.js
@@ +593,3 @@
> if (this.isOuterHTMLEditable && isSelectionElement) {
> editHTML.removeAttribute("disabled");
> + pasteOuterHTML.removeAttribute("disabled");
It would be great here if we could disable the context menu item if the clipboard is invalid to prevent any confusion for the user. Since we also check this later, you could make a function like _getClipboardContentForOuterHTML that returns null when the content is invalid (non html/text, empty string, etc), and use it in both places. Then you could set pasteOuterHTML as disabled if this was the case.
@@ +713,5 @@
> this.markup.beginEditingOuterHTML(this.selection.nodeFront);
> }
> },
>
> + pasteOuterHTML: function InspectorPanel_pasteOuterHTML()
Add a header comment for this function like copyInnerHTML has
@@ +715,5 @@
> },
>
> + pasteOuterHTML: function InspectorPanel_pasteOuterHTML()
> + {
> + let clipboard = require("sdk/clipboard");
Move this require line up to the top of the file next to the others like CssLogic
@@ +720,5 @@
> + let flavors = clipboard.currentFlavors;
> + if (this.selection.isNode() &&
> + (flavors.indexOf("html") != -1 || flavors.indexOf("text") != -1)) {
> + let node = this.selection.nodeFront;
> + this.markup.updateNodeOuterHTML(node, clipboard.get(), node.outerHTML);
Instead of accessing node.outerHTML here, we need to fetch this from the server like so:
let node = this.selection.nodeFront;
this.markup.getNodeOuterHTML(node).then((oldValue)=> {
this.markup.updateNodeOuterHTML(node, clipboard.get(), oldValue);
});
Attachment #8405964 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 14•11 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> For info, you can follow the bug about the mach test command here: bug 995972
That bug just got fixed in fx-team, you will now be able to run your tests with `mach mochitest-devtools something/something/something.js`
Assignee | ||
Comment 15•11 years ago
|
||
Hi Patrick,
Sorry about the delay. So far I've made all the changes you suggested except for the tests but I ran into an issue related to undoing the paste (I think). When I do a paste outer HTML, everything works as expected. But when I do a Cmd-Z, I get the following in the console:
console.error:
Message: TypeError: node is null
Stack:
WalkerActor<.setOuterHTML<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1727:1
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:903:1
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1110:9
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:7
WalkerActor<.setOuterHTML<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1727:1
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:903:1
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1110:9
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:7
console.error:
unknownError
I can only duplicate this using the keyboard, since the undo menu item is disabled. From the error message, I'm guessing it's because I've completely replaced the node with something else. This also happens when I edit as HTML.
Since it also happens with edit as HTML, I'm not going to solve this here. Maybe a bug should be created for it?
As soon as I get the tests written, I will upload again.
Assignee | ||
Comment 16•11 years ago
|
||
The above comment was supposed to be addressed to Brian, not Patrick. Sorry for the confusion!
Comment 17•11 years ago
|
||
(In reply to Gorman Ho from comment #15)
> Hi Patrick,
>
> Sorry about the delay. So far I've made all the changes you suggested except
> for the tests but I ran into an issue related to undoing the paste (I
> think). When I do a paste outer HTML, everything works as expected. But when
> I do a Cmd-Z, I get the following in the console:
>
> console.error:
> Message: TypeError: node is null
> Stack:
>
> WalkerActor<.setOuterHTML<@resource://gre/modules/commonjs/toolkit/loader.js
> -> resource://gre/modules/devtools/server/actors/inspector.js:1727:1
> actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/protocol.js:903:1
> DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1110:9
> LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm ->
> resource://gre/modules/devtools/server/transport.js:258:11
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/DevToolsUtils.js:82:7
>
> WalkerActor<.setOuterHTML<@resource://gre/modules/commonjs/toolkit/loader.js
> -> resource://gre/modules/devtools/server/actors/inspector.js:1727:1
> actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/protocol.js:903:1
> DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1110:9
> LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm ->
> resource://gre/modules/devtools/server/transport.js:258:11
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/DevToolsUtils.js:82:7
> console.error:
> unknownError
>
> I can only duplicate this using the keyboard, since the undo menu item is
> disabled. From the error message, I'm guessing it's because I've completely
> replaced the node with something else. This also happens when I edit as HTML.
>
> Since it also happens with edit as HTML, I'm not going to solve this here.
> Maybe a bug should be created for it?
>
> As soon as I get the tests written, I will upload again.
Thanks for the info, and don't worry about the undo issue - we will handle that in Bug 998933.
Assignee | ||
Comment 18•11 years ago
|
||
Since I'm just calling MarkupView.updateNodeOuterHTML(), the only thing I really need to test is whether the menu item is accessible for various types of clipboard content. I think the best place for that is in browser_inspector_menu.js, since that's where all the other menu items are tested. Let me know what you think!
Attachment #8405964 -
Attachment is obsolete: true
Attachment #8413317 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8413317 [details] [diff] [review]
993416.patch
Review of attachment 8413317 [details] [diff] [review]:
-----------------------------------------------------------------
This may be a bug, but calling clipboard.set("") doesn't do what I expect it to do. After calling it, clipboard.get() returns an empty string, but with length 1! The only way I could clear the clipboard such that length is 0 is if I restart the computer. Not sure if this is specific to OS X.
Comment 20•11 years ago
|
||
Comment on attachment 8413317 [details] [diff] [review]
993416.patch
Review of attachment 8413317 [details] [diff] [review]:
-----------------------------------------------------------------
Nice! I've made a couple of notes below, let me know if you have any questions.
::: browser/devtools/inspector/inspector-panel.js
@@ +551,5 @@
> + _getClipboardContentForOuterHTML: function Inspector_getClipboardContentForOuterHTML() {
> + let flavors = clipboard.currentFlavors;
> + if (flavors.indexOf("text") != -1 ||
> + (flavors.indexOf("html") != -1 && flavors.indexOf("image") == -1)) {
> + return clipboard.get();
Your comment about clipboard.set("") causing clipboard.get() to return a string of length 1 makes me wonder if we should also be checking that the string is not *just* whitespace. In this case, it seems like it would be good if the menu item was disabled since the effect would be the same as doing a "Delete Node".
clipboard.get() can be null, so it would have to be something like:
let clipboardData = clipboard.get();
// Treat whitespace only content as null
if (clipboardData && clipboardData.trim().length === 0) {
clipboardData = null;
}
If we did this, then you could add two more test cases, one with an empty string (which may or may not end up turning into a length=1 string), and one with a whitespace-only string like " \n\t\n\n"
::: browser/devtools/inspector/test/browser_inspector_menu.js
@@ +69,5 @@
> checkEnabled("node-menu-pseudo-" + name);
> }
>
> + checkElementPasteOuterHTMLMenuItemForText();
> + //testCopyInnerMenu();
Looks like the testCopyInnerMenu test got commented out
@@ +74,4 @@
> });
> }
>
> + let clipboard = require("sdk/clipboard");
Please move this clipboard require to the top of the test() function
@@ +74,5 @@
> });
> }
>
> + let clipboard = require("sdk/clipboard");
> +
I like your test changes. I think we should also test triggering a command event on the context menu item (if it is enabled) and make sure the node's outerHTML gets set as we expect before proceeding to the next test.
For triggering the command event, see the testDeleteNode function in this file.
As for waiting for the outerHTML to be set, we should be able to wait for `inspector.selection.once("new-node")` and check something simple like this:
doc.body.outerHTML.contains(data);
The actual outerHTML editing is tested much more thoroughly elsewhere - we just need to make sure the context menu command is working.
Attachment #8413317 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8413317 -
Attachment is obsolete: true
Attachment #8417002 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 22•11 years ago
|
||
Thanks for all your feedback! Please let me know if I've missed anything in my latest patch.
Comment 23•11 years ago
|
||
Comment on attachment 8417002 [details] [diff] [review]
993416.patch
Review of attachment 8417002 [details] [diff] [review]:
-----------------------------------------------------------------
This looks great! Please fix a typo on commit message: "r=pgrins" should be "r=bgrins".
::: browser/devtools/inspector/test/browser_inspector_menu.js
@@ +163,5 @@
> + event.initCommandEvent("command", true, true, window, 0, false, false,
> + false, false, null);
> + menu.dispatchEvent(event);
> + inspector.selection.once("new-node", (event, oldNode, newNode) => {
> + ok(doc.body.outerHTML.contains(clipboard.get()),
Add one more assertion here just to make sure it worked on the correct element:
ok(!doc.querySelector("h1"), "The h1 has been removed");
Attachment #8417002 -
Flags: review?(bgrinstead)
Comment 24•11 years ago
|
||
I've pushed this to the test server: https://tbpl.mozilla.org/?tree=Try&rev=1d285a9a0bdc
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #8417002 -
Attachment is obsolete: true
Attachment #8418516 -
Flags: review?(bgrinstead)
Comment 26•11 years ago
|
||
Comment on attachment 8418516 [details] [diff] [review]
993416.patch
Review of attachment 8418516 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Let's wait for a green try before marking checkin-needed: https://tbpl.mozilla.org/?tree=Try&rev=1e62e2431577
Attachment #8418516 -
Flags: review?(bgrinstead) → review+
Comment 27•11 years ago
|
||
Tests look green, marking checkin-needed
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee | ||
Comment 28•11 years ago
|
||
That's great news! Thanks again for all your help. Much appreciated. :)
Comment 29•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=bgrins][good first bug][lang=js] → [mentor=bgrins][good first bug][lang=js][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][good first bug][lang=js][fixed-in-fx-team] → [mentor=bgrins][good first bug][lang=js]
Target Milestone: --- → Firefox 32
Comment 31•11 years ago
|
||
Will, this bug added a new 'paste outer html' context menu item in the markup view for pasting outer HTML into the selected node. It is just as if you did an 'edit as html', selected everything and replaced it with your clipboard contents, but it saves a few steps. It is disabled if your clipboard is empty or has an image in it.
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
Comment 32•11 years ago
|
||
Thanks bgrins! I'll update the docs for Firefox 32 to include this.
Flags: needinfo?(wbamberg)
Updated•10 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•