Closed Bug 968241 Opened 10 years ago Closed 9 years ago

[markup view] "Copy" keyboard shortcut should copy the outer HTML of the selected node onto the clipboard

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: bgrins, Assigned: julian.descottes, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=html][good first bug][lang=js])

Attachments

(1 file, 8 obsolete files)

When I press cmd/ctrl+c, it would be great if the outer HTML of the selected node was added to my clipboard.  This option is already available via a context menu item, I would just like it to be accessible from a keyboard shortcut.
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js]
(In reply to vikneshwar from comment #6)
> Hai Brian , 
>   Can i get this bug assigned . thank you :)

Sure, let's start on it.  I will find another bug for Zulfa once I hear back.  OK, so here is the basic plan:

There is already a function on the inspector panel called copyOuterHTML [0].

So what we need to do is add a case to the markup view _onKeyDown function, listening for a copy key event [1].  For reference, this is done similarly in the variables view by checking for (e.keyCode == e.DOM_VK_C && (e.ctrlKey || e.metaKey)) [2].

Once we have detected the copy key sequence in the markup view, we just need to call this._inspector.copyOuterHTML(), and it should be on the clipboard.


[0]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#727
[1]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#322
[2]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm#892
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
Flags: needinfo?(zulfa2all)
Comment on attachment 8374773 [details] [diff] [review]
outerhtml.patch

Review of attachment 8374773 [details] [diff] [review]:
-----------------------------------------------------------------

I realized there were some edge cases I forgot about.  For instance, if you control+c on a comment node it throws an error right now.  We will need to detect the node type in the copyOuterHTML function and change what we put on the clipboard in this case.


By checking on this.selection.nodeFront.nodeType we can detect the different types.  The main ones we care about are element, doctype and comments: https://developer.mozilla.org/en-US/docs/Web/API/Node.nodeType

Here is a sample of what it will look like in the copyOuterHTML function: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#727.

    let node = this.selection.nodeFront;
    if (node.nodeType === 1) { // Element
      this._copyLongStr(this.walker.outerHTML(node));
    } else if (node.nodeType === 8) { // Comment

    } else if (node.nodeType === 10) { // Doctype

    }

1) The comment can be grabbed with nodeValue using this boilerplate code

    node.getNodeValue().then(longstr => {
      longstr.string().then(nodeValue => {
        longstr.release().then(null, console.error);

        // Now you have the comment text
      });
    });


2) The doctype is already being built as a string in the markup view: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1597.  What we should do here is move this logic into a getter on the actor in the file toolkit/devtools/server/actors/inspector.js so it can be reused.  Then we can use doctypeString in both the markup view, and in our copyOuterHTML function.

See: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#219.

    // doctype attributes
    name: this.rawNode.name,
    publicId: this.rawNode.publicId,
    systemId: this.rawNode.systemId,
    get doctypeString() {
      // Return the doctype string here
    },

You can test out your changes on https://www.mozilla.org/en-US/, which has both comment and doctype nodes visible in the markup view.
Attachment #8374773 - Flags: review?(bgrinstead)
Attached patch outerhtml_1.patch (obsolete) — Splinter Review
Hai Brian , 
  Made changes as mentioned in previous comment. But not sure whether it works . . Please suggest what improvements i need to make . Thank you :)
Attachment #8375649 - Flags: review?(bgrinstead)
Comment on attachment 8375649 [details] [diff] [review]
outerhtml_1.patch

Review of attachment 8375649 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/inspector/inspector-panel.js
@@ +732,5 @@
>      }
> +    var node=this.selection.nodeFront;
> +    if(node.nodeType == 1){
> +      this._copyLongStr(this.walker.outerHTML(this.selection.nodeFront));
> +    }else if(node.nodeType == 8){

Add extra whitespace here:

} else if(node.nodeType == 8) {

@@ +735,5 @@
> +      this._copyLongStr(this.walker.outerHTML(this.selection.nodeFront));
> +    }else if(node.nodeType == 8){
> +      node.getNodeValue().then(longstr => {
> +        longstr.string().then(nodeValue => {
> +          longstr.release().then(null,console.error)

You need to copy the nodeValue variable onto the clipboard now

this._copyLongStr(nodeValue);

@@ +739,5 @@
> +          longstr.release().then(null,console.error)
> +          });
> +        });
> +    }else if(node.nodeType == 10){
> +      doctypeString()

Missing a bracket:

else if (node.nodeType == 10) {
  this._copyLongStr(node.doctypeString);
}

::: browser/devtools/markupview/markup-view.js
@@ +1599,5 @@
>   */
>  function DoctypeEditor(aContainer, aNode) {
>    this.elt = aContainer.doc.createElement("span");
>    this.elt.className = "editor comment";
> +  doctypeString();

this.elt.textContent = aNode.doctypeString

::: toolkit/devtools/server/actors/inspector.js
@@ +220,5 @@
>        name: this.rawNode.name,
>        publicId: this.rawNode.publicId,
>        systemId: this.rawNode.systemId,
> +      get doctypeString(){
> +        this.elt.textContent = '<!DOCTYPE '+ aNode.name + (aNode.publicId ? ' PUBLIC "' + aNode.publicId + '"':'') + (aNode.systemId ? ' "' + aNode.systemId + '"':'') + '>';

You need to close the bracket

get doctypeString(){
    return '<!DOCTYPE '+ this.name + (this.publicId ? ' PUBLIC "' + this.publicId + '"':'') + (this.systemId ? ' "' + this.systemId + '"':'') + '>';
},
Attachment #8375649 - Flags: review?(bgrinstead)
Attached patch outerhtml_2.patch (obsolete) — Splinter Review
Made Changes , still din't work . Can't figure out whats wrong :(
Attachment #8375689 - Flags: review?(bgrinstead)
Comment on attachment 8375689 [details] [diff] [review]
outerhtml_2.patch

Review of attachment 8375689 [details] [diff] [review]:
-----------------------------------------------------------------

Do you know how to run Firefox with the browser console opened?  I ask because there are still runtime errors here that should be pretty easy to track down if you see the error logs.

Make sure you have checked 'Enable chrome debugging' and 'Enable remote debugging' in the options panel in devtools, then open up the browser console with tools -> web developer -> browser console (or ctrl/cmd+shift+J).

Now you should see any errors when you try to open devtools.

::: toolkit/devtools/server/actors/inspector.js
@@ +1,2 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 1.0. If a copy of the MPL was not distributed with this

Not sure why this line changed? Please don't change the license block

@@ +220,5 @@
>        name: this.rawNode.name,
>        publicId: this.rawNode.publicId,
>        systemId: this.rawNode.systemId,
> +      get doctypeString() {
> +        this.elt.textContent = '<!DOCTYPE '+ aNode.name + (aNode.publicId ? ' PUBLIC "' + aNode.publicId + '"':'') + (aNode.systemId ? ' "' + aNode.systemId + '"':'') + '>';

You need to return the string here instead of setting text content, and not use `aNode` but `this` instead
Attachment #8375689 - Flags: review?(bgrinstead)
Attached patch outerhtml_3.patch (obsolete) — Splinter Review
Made changes and got this error "InspectorPanel is not defined" in main.js even though it has been imported using require() from inspector-panel.js . Please help me to identify what i am doing wrong. Thank you :)

(sorry for my bad English)
Attachment #8376846 - Flags: review?(bgrinstead)
Attachment #8375649 - Attachment is obsolete: true
Attachment #8375689 - Attachment is obsolete: true
Attachment #8376846 - Flags: review?(bgrinstead)
Priority: -- → P2
Assignee: nobody → amanjain110893
Mentor: bgrinstead
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js]
> With my team, we study, at the moment, the project architecture to describe
> this. Watching this, we understood yesterday morning that the bug can be
> corrected if we concentrate into browser/devtools/markupview and
> toolkit/devtools directories. Is it correct?

The files that will need to change are change browser/devtools/inspector/inspector-panel.js and browser/devtools/markupview/markup-view.js, and toolkit/devtools/server/actors/inspector.js.  Please take a look at comment 7 and comment 9 for an explanation of what should happen in each.

Also, the existing patch attached to this bug could be valuable for a reference.  I don't think it is working as-is, but it's on the right path.
Alexis, you can search for these files on this site: http://dxr.mozilla.org/mozilla-central/source/.

As an example for inspector-panel.js, you can search for inspector-panel and see these results: http://dxr.mozilla.org/mozilla-central/search?q=inspector-panel&redirect=true.  You can see that this is referenced here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/main.js#26, which is then using it to actually load the main JS file for the Inspector tab in the toolbox.

Basically, inspector-panel.js and markup-view.js are on the frontend, loaded in the Inspector tab in the toolbox.

inspector.js is a little more tricky - the NodeActor is more or less running inside of the page you are debugging and communicates with the frontend through a remote protocol.  But the scope of the change in inspector.js for this bug is pretty small, and you shouldn't have to deal with the protocol directly.
Attachment #8605753 - Flags: review+ → review?(bgrinstead)
Comment on attachment 8605753 [details] [diff] [review]
copyOuterHTML5.patch

Review of attachment 8605753 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right start.  It needs to also handle comment and doctype nodes - see Comment 9 and https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=968241&attachment=8376846, which is already handling these cases.

Also, we will need a test case for this feature.  I think it'd be OK to modify the following file to include support for the keyboard event: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_menu-02-copy-items.js.  Basically, add another loop below the current one that will look something like this (I haven't tested it so it will probably need some modifications)


  function sendCopyKey() {
    EventUtils.synthesizeKey("C", { accelKey: true }, inspector.panelWin);
  }

  for (let {desc, id, selector, text} of COPY_ITEMS_TEST_DATA) {
    info("Testing " + desc);
    yield selectNode(selector, inspector);

    let deferred = promise.defer();
    waitForClipboard(text, sendCopyKey,
                     deferred.resolve, deferred.reject);
    yield deferred.promise;
  }

To run this locally, you can run `./mach mochitest-devtools browser/devtools/inspector/test/browser_inspector_menu-02-copy-items.js`
Attachment #8605753 - Flags: review?(bgrinstead)
Fly-by comment: the current patch will copy the node outer HTML even if ctrl/cmd isn't pressed, so just pressing C will work. I think we want the copy shortcut to work only, so cmd+C or ctrl+C.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #56)
> Fly-by comment: the current patch will copy the node outer HTML even if
> ctrl/cmd isn't pressed, so just pressing C will work. I think we want the
> copy shortcut to work only, so cmd+C or ctrl+C.

Good point.  I guess we need to check aEvent.ctrlKey || aEvent.metaKey
Good morning,

Tanks for your suggestions. :)

I will try it and I will informed you.

However, I have a question: it's impossible, for the moment, to copy doctypes and comments nodes even if we use mouse. So, do you want that I try to resolve this problem too or only with the cmd ctrl-c?

Have a nice day.

Best regards :)

Alexis Zurawska
(In reply to zurawska.alexis from comment #58)
> However, I have a question: it's impossible, for the moment, to copy
> doctypes and comments nodes even if we use mouse. So, do you want that I try
> to resolve this problem too or only with the cmd ctrl-c?

Don't worry about that.  This patch will add the ability to copy those nodes and we can file a follow up bug to enable the context menu item.
See Also: → 1169771
Assignee: zurawska.alexis → nobody
Status: ASSIGNED → NEW
Hi Brian,

I would like to work on this one, can you assign it to me ?
Flags: needinfo?(bgrinstead)
(In reply to Julian Descottes from comment #61)
> I would like to work on this one, can you assign it to me ?

Yeah, sounds good!  See Comment 7, Comment 9, and Comment 19 as a starting point.
Assignee: nobody → julian.descottes
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Attached patch Bug968241.patch (obsolete) — Splinter Review
Ended up using the copy event instead of CTRL+C because I had troubles simulating CTRL+C in the mochitests.

If that's a concern I can give it another try, I'm not 100% sure the issues I had were linked to the event creation.
Attachment #8628568 - Flags: review?(bgrinstead)
Comment on attachment 8628568 [details] [diff] [review]
Bug968241.patch

Review of attachment 8628568 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!  I'm going to be unavailable for a few days so I'm going to forward the review to Patrick
Attachment #8628568 - Flags: review?(bgrinstead) → review?(pbrosset)
Mentor: pbrosset
Attachment #8376846 - Attachment is obsolete: true
Attachment #8605753 - Attachment is obsolete: true
Comment on attachment 8628568 [details] [diff] [review]
Bug968241.patch

Review of attachment 8628568 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks. I have a few comments in the code below, the most important being about the server-side changes.
One more thing though: it's currently possible (albeit hard) to select text in the markup-view with the mouse. It's hard because it somehow sometimes conflicts with dragging nodes to re-order them in the tree, but sometimes you can do it. When you do, you'd sort of expect Ctrl+C to actually copy the selected text, not the outer HTML of the currently selected node.
2 solutions:
a - either we continue to make this work by first checking if the document has an active text selection,
b - or we kill the text selection feature.

I'd go for (b) because:
- selecting text is hard and conflicts with the drag-drop of nodes
- at least Chrome doesn't do it
- copying outer HTML of nodes is more powerful and useful. Why would anyone want to copy a malformed part of the HTML? If you want to copy an attribute you can simply dbl-click it.
- it can be done far more easily in the view-source screen
- if we do this, we could get rid of the delay that's used currently in the node drag-drop feature.

I think going for (b) is slightly more work (maybe?), but better on the long run. I'm ok if you want to go for (a) and if you file a follow-up bug to do (b).

::: browser/devtools/inspector/inspector-panel.js
@@ +1083,5 @@
>        container.copyImageDataUri();
>      }
>    },
>  
> +  _getLongStr: function(promise) {

Could you add a jsdoc comment block here and before _copyLongStr too that explains that the promise passed as a parameter is expected to yield a LongString Actor (/toolkit/devtools/server/actors/string.js)

@@ +1089,5 @@
> +      return longstr.string().then(str => {
> +        longstr.release().then(null, console.error);
> +        return str;
> +      });
> +    }).then(null, console.error);

I think you can replace
.then(null, console.error);
with
.catch(console.error);

Same below in _copyLongStr.

I'm wondering if console.error works however, shouldn't it be console.error.bind(console) ?
We have another error reporting mechanism that's used a lot in devtools code:

Cu.reportError
I suggest you use this instead.

::: browser/devtools/inspector/test/browser.ini
@@ +25,3 @@
>    doc_inspector_remove-iframe-during-load.html
>    doc_inspector_search.html
> +  doc_inspector_search.html

This entry is duplicated.

::: browser/devtools/inspector/test/browser_inspector_keyboard-shortcuts-copy-outerhtml.js
@@ +10,5 @@
> +add_task(function *() {
> +  let { inspector } = yield openInspectorForURL(TEST_URL);
> +  let root = inspector.markup._elt;
> +
> +  info('Test copy outerHTML for COMMENT node');

nit: please wrap strings in double-quotes instead.

@@ +18,5 @@
> +
> +  info('Test copy outerHTML for DOCTYPE node');
> +  let doctype = getElementByType(inspector, Ci.nsIDOMNode.DOCUMENT_TYPE_NODE);
> +  yield setSelectionNodeFront(doctype, inspector);
> +  yield checkClipboard("<!DOCTYdPE html>", root);

s/DOCTYdPE/DOCTYPE

@@ +25,5 @@
> +  yield selectAndHighlightNode("div", inspector);
> +  yield checkClipboard("<div><p>Test copy OuterHTML</p></div>", root);
> +});
> +
> +function *setSelectionNodeFront(node, inspector) {

nit: we tend to place the start next to function instead. I know this changes nothing, but it would make it more consistent:
function* setSelectionNodeFront

@@ +49,5 @@
> +  }
> +}
> +
> +function getElementByType(inspector, type) {
> +  for (let [node, container] of inspector.markup._containers) {

container isn't used here, so this would be enough:
for (let [node] of inspector.markup._containers)

@@ +54,5 @@
> +    if (node.nodeType === type) {
> +      return node;
> +    }
> +  }
> +}
\ No newline at end of file

nit: please add an empty line at the end of the file.

::: toolkit/devtools/server/actors/inspector.js
@@ +244,5 @@
>        // doctype attributes
>        name: this.rawNode.name,
>        publicId: this.rawNode.publicId,
>        systemId: this.rawNode.systemId,
> +      doctypeString: this.doctypeString,

This is useful to have on the NodeActor, but this is unfortunately going to cause a backwwards compatibility issue. Imagine the scenario where one uses a build of Firefox with this patch in to debug another, older, build of Firefox.
We'd end up with a recent version of the code on the client-side and an older version of the code on the server-side (which we support) and so doctypeString won't be found.

For info, because this isn't obvious when working with actor modules at first: the module is loaded once on the server, and once on the client. So that the server can run the Actor, and the client can run the associated Front class. But they both load the module from the file that was packaged in their build.

So, the thing here is that the doctypeString can be built from name, publicId and systemId, all of these are already sent by the actor. So I suggest removing this new doctypeString from the form here, then removing the getter you added below in the actor class.
Then keep the new doctypeString getter you added in the front class, but make it return the concatenated string :

get doctypeString() {
  return '<!DOCTYPE ' + this.name +
         (this.publicId ? ' PUBLIC "' +  this.publicId + '"': '') +
         (this.systemId ? ' "' + this.systemId + '"' : '') +
         '>';
},
Attachment #8628568 - Flags: review?(pbrosset) → feedback+
Thanks ! 

> I think going for (b) is slightly more work (maybe?), but better on the long run. 
> I'm ok if you want to go for (a) and if you file a follow-up bug to do (b).

I agree, let's prevent other copy actions. I think I can simply prevent default in my copy handler ?

> I'm wondering if console.error works however, shouldn't it be console.error.bind(console) ?

Regarding console.error, it looks like it's linked to Bug 989619. console.error used to work with any scope, but it's no longer the case. In some classes it is manually bound, for instance browser/devtools/webide/content/newapp.js.

A quick search on "then(null, console.error);" returns quite a lot of matches, should we open a bug for cleanup or do it here ?
(In reply to Julian Descottes from comment #66)
> A quick search on "then(null, console.error);" returns quite a lot of
> matches, should we open a bug for cleanup or do it here ?
Yes, please file a bug (in the general Developer Tools component)
Attached patch Bug968241-review.patch (obsolete) — Splinter Review
Only attached what changed since my previous patch. Let me know if you want me to upload a patch with a single commit instead.
Attachment #8629040 - Flags: review?(pbrosset)
Attached patch Bug968241-review.patch (obsolete) — Splinter Review
Forgot to remove the duplicate entry in browser.ini in the previous patch.
Attachment #8629040 - Attachment is obsolete: true
Attachment #8629040 - Flags: review?(pbrosset)
Attachment #8629066 - Flags: review?(pbrosset)
As discussed, merged the commits (and rebased as well).
Attachment #8628568 - Attachment is obsolete: true
Attachment #8629066 - Attachment is obsolete: true
Attachment #8629066 - Flags: review?(pbrosset)
Attachment #8629095 - Flags: review?(pbrosset)
Comment on attachment 8629095 [details] [diff] [review]
Bug968241-merged.patch

Review of attachment 8629095 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, all my comments have been addressed. I'll push to try shortly.
Attachment #8629095 - Flags: review?(pbrosset) → review+
Good afternoon Patrick,

Thanks for the review & validation !
https://hg.mozilla.org/mozilla-central/rev/0757f3c684f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
I've noted this in the keyboard shortcuts page for the Inspector, and in the docs for the Inspector's popup menu:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_menu

Please let me know if you think anything else is needed. Thanks!
Flags: needinfo?(julian.descottes)
In https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts the MacOSX shortcut should be "Cmd+C" instead of "Ctrl+C". I just updated https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts accordingly, hope that's fine.

Other than that, looks good to me. Thanks for writing the documentation !
Flags: needinfo?(julian.descottes)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.