Closed Bug 993416 Opened 10 years ago Closed 10 years ago

[markup view] Context menu to paste HTML in a node

Categories

(DevTools :: Inspector, defect, P2)

defect

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)

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)
Given what we have in place, this looks like a good-first-bug
Priority: -- → P2
Whiteboard: [good first bug][lang=js]
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]
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.
Attached patch 993416.patch (obsolete) — Splinter Review
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.
(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).
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
Thanks for assigning the bug and for the advice. One more question: how do I run the tests in browser_inspector_menu.js?
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.
For info, you can follow the bug about the mach test command here: bug 995972
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)
(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`
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.
The above comment was supposed to be addressed to Brian, not Patrick. Sorry for the confusion!
Depends on: 998933
(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.
Attached patch 993416.patch (obsolete) — Splinter Review
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)
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 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)
Attached patch 993416.patch (obsolete) — Splinter Review
Attachment #8413317 - Attachment is obsolete: true
Attachment #8417002 - Flags: review?(bgrinstead)
Thanks for all your feedback! Please let me know if I've missed anything in my latest patch.
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)
Attached patch 993416.patchSplinter Review
Attachment #8417002 - Attachment is obsolete: true
Attachment #8418516 - Flags: review?(bgrinstead)
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+
Tests look green, marking checkin-needed
Status: NEW → ASSIGNED
Keywords: checkin-needed
That's great news! Thanks again for all your help. Much appreciated. :)
https://hg.mozilla.org/integration/fx-team/rev/79f854e8a883
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]
https://hg.mozilla.org/mozilla-central/rev/79f854e8a883
Status: ASSIGNED → RESOLVED
Closed: 10 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
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
Thanks bgrins! I'll update the docs for Firefox 32 to include this.
Flags: needinfo?(wbamberg)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.