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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Zulfa]

Hi Brian
I am interested in working on this bug.  Can I be assigned to it ?

Comment 2

[Zulfa]

As my first bug to tackle and contribute on Mozilla, can you direct me where to start.
thanks

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Zulfa from comment #2)
> As my first bug to tackle and contribute on Mozilla, can you direct me where
> to start.
> thanks

Sure!  Take a look at https://wiki.mozilla.org/DevTools/GetInvolved for some information and resources about working on DevTools.  Once you are ready to start pulling down the code, follow the instructions at https://wiki.mozilla.org/DevTools/Hacking#First_Build.  Feel free to hop onto the #devtools IRC channel and ask if you have any issues getting set up.

Once you have a working build, let me know and we can talk about how to tackle this bug.

Comment 4

[vikneshwar]

Hi Brian 
I would like to work on this bug . This is my second bug . My first bug is bugzilla.mozilla.org/show_bug.cgi?id=957072 . Thank you :)

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to vikneshwar from comment #4)
> Hi Brian 
> I would like to work on this bug . This is my second bug . My first bug is
> bugzilla.mozilla.org/show_bug.cgi?id=957072 . Thank you :)

Zulfa, just checking in to see if you had any luck getting the project building?

Comment 6

[vikneshwar]

Hai Brian , 
  Can i get this bug assigned . thank you :)

Comment 7

[Brian Grinstead [:bgrins]]

(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

Comment 8

[vikneshwar]

Attachment: outerhtml.patch

Hai Brian , 
      I think it works . Check it and review it . Thank you :)

Comment 9

[Brian Grinstead [:bgrins]]

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.

Comment 10

[vikneshwar]

Attachment: outerhtml_1.patch

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 :)

Comment 11

[Brian Grinstead [:bgrins]]

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 + '"':'') + '>';
},

Comment 12

[vikneshwar]

Attachment: outerhtml_2.patch

Made Changes , still din't work . Can't figure out whats wrong :(

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[vikneshwar]

Attachment: outerhtml_3.patch

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)

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8374773 [details] [diff] [review]
outerhtml.patch

Marking older patches as obsolete.

Comment 16

[Manish Goregaokar [:manishearth]]

(In reply to vikneshwar from comment #14)
> Created attachment 8376846 [details] [diff] [review]
> outerhtml_3.patch
> 
> 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)

Have you uploaded the correct patch? InspectorPanel is *defined* in inspector-panel.js, and I don't see any new references in the patch.

Comment 17

[Patrick Brosset <:pbro>]

No activity for more than a month, putting back in the backlog.

Comment 18

[amanjain110893]

I am want to start with firefox dev tools. Can someone please assigned this bug to me, if its not yet fixed. I want to solve bugs related to UI and also I want to learn about and work with systems, can someone help me.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to amanjain110893 from comment #18)
> I am want to start with firefox dev tools. Can someone please assigned this
> bug to me, if its not yet fixed. I want to solve bugs related to UI and also
> I want to learn about and work with systems, can someone help me.

Hello, please take a look at the discussion leading up to now and the existing patch (Attachment 8376846 [details] [diff]).  The ideas in the patch are right, but I believe that there are still some errors in it right now.

Comment 20

[zurawska.alexis]

Hi !

There are four of students of the Paris VIII university which is situated in Saint-Denis close to Paris.
We want to work on this bug in a project class named "Program Comprhension" (the name of the course).

Our names are Alexis (myself), Hacene, Nassim and Axel.

Have a nice day and a nice week-end.:)

Comment 21

[Manish Goregaokar [:manishearth]]

I guess you guys should go through the existing patch and the comments in this thread, especially comment #7 . Leave a comment or ping me/bgrins on IRC if you need further help!

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to zurawska.alexis from comment #20)
> There are four of students of the Paris VIII university which is situated in
> Saint-Denis close to Paris.
> We want to work on this bug in a project class named "Program Comprhension"
> (the name of the course).
> 
> Our names are Alexis (myself), Hacene, Nassim and Axel.

Sure, I'll reassign the bug to you.  The first step is getting a Firefox build running (see Comment 3)

Comment 23

[zurawska.alexis]

(In reply to Brian Grinstead)

Thank you to assign this bug to my team. I'm sorry if I ask to the comment too much time after your answer but we have other courses and homeworks to effecutate.

I asked to my team to create an individual account to can discuss together with your team.

Before we begin to explore link that you have indicated, I ask questions to my teacher.

Best reguards :)

Comment 24

[zurawska.alexis]

Good evening to everybody,

I'm sorry to ask to you after a very long time without news.

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?

We will begin the correction of the bug only the next mounth.

I give news to you nextly.

Best reguards :)

Comment 25

[Brian Grinstead [:bgrins]]

> 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.

Comment 26

[axel peyrat]

hello i'm axel student of saint denis and work on the team of alexis zurawska.
actualy we study the files which you indicate to us.
we must describe these and we will begin to correct bug (on the first week of december because we have two weeks to describe files, beginning today) 
we will give a new about it

best reguards ^.^

Comment 27

[zurawska.alexis]

Good evening,

With my team, we would know how to compile code after we have change something because we describe files that you indicate to us and we try to debug as the same time. Thanks.

Best regards :)

Alexis Zurawska

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to zurawska.alexis from comment #27)
> Good evening,
> 
> With my team, we would know how to compile code after we have change
> something because we describe files that you indicate to us and we try to
> debug as the same time. Thanks.

Please see this article for more info on how to build and see the changes: https://wiki.mozilla.org/DevTools/Hacking#Development_workflow.

Comment 29

[zurawska.alexis]

Thanks for this precious information Brian.

Today, we advanced about files description. We should begin changes two weeks later (friday, December, 19th).

We give you news.

Best regards :)

Alexis Zurawska

Comment 30

[zurawska.alexis]

Good evening, 

We would have more informations about files interaction to can realize a work which consist in the description of files which are implicates in this bug. At the moment, we enumerated the sources that these files uses but we would know which files use these three files (inspector.js, inspector-panel.js and markup-view.js).

Thanks.

Best regards :)

Alexis Zurawska

Comment 31

[Brian Grinstead [:bgrins]]

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.

Comment 32

[zurawska.alexis]

Thanks you to your help Brian. Your links are very good and I realize a grep -lr in main directory of Mozilla project to each String concerned. I believe that it's sufficient to do our graph.

Have a nice day.

Best regards :)

Alexis Zurawska

Comment 33

[zurawska.alexis]

Good evening Brian,

I have maybe a sully question but I would know if the patch was directly integrated to my version that I downloaded in october or if we must integrate it manually.

Have a nice new year eve.

Best regards :)

Alexis Zurawska

P.S: We are sorry but we taked delay to correct this bug. We will begin next monday and we hope to succeed the correction bug. We give you news nextly.

Comment 34

[Brian Grinstead [:bgrins]]

(In reply to zurawska.alexis from comment #33)
> Good evening Brian,
> 
> I have maybe a sully question but I would know if the patch was directly
> integrated to my version that I downloaded in october or if we must
> integrate it manually.

You will have to apply it manually.  Also, make sure you've updated your source tree and rebuilt recently because those files have changed quite a bit in the last couple of months.  

Documentation about how to create and apply patches is here: https://developer.mozilla.org/en-US/docs/Mercurial_Queues.  You could either:
1) Apply the existing patch to your code base using mq using the qimport command.
2) Make a new patch locally (with qnew) and simply find the relevant parts in the files and copy the changes over.

In this case, option 2 may be easier, since there aren't many changes and there are going to be merge conflicts if you try to apply the patch.

Comment 35

[zurawska.alexis]

Good evening Brian,

To begin, I hope to you a happy new year and very much health and happiness for 2015.

Thanks you for your precious help.

Best regards :)

Alexis Zurawska

Comment 36

[zurawska.alexis]

Good evening Brian, 

I apply that you say to me but when I want apply the existing patch in my code, I have this message: unable to read file outerhtml_3. What can I do?

Thanks.

Best regards :)

Alexis Zurawska

Comment 37

[Brian Grinstead [:bgrins]]

As I said, you will have to deal with merge conflicts if you go this way, but you can run `hg qimport https://bug968241.bugzilla.mozilla.org/attachment.cgi?id=8376846` and not have to worry about pulling in the file from your hard drive.

I think it would be easier overall to start with a new patch and copy over the relevant changes that you see from https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=968241&attachment=8376846, but either way would work.

Comment 38

[zurawska.alexis]

Good evening Brian,

My co-mates Hacène and Nassim work actually on the bug correction.

I will ask to them to create an account as Axel and me to give you news about advancement of bug correction.

In fact, I succeeded to import patch with your command. Thanks :)

Best regards,

Alexis Zurawska

Comment 39

[zurawska.alexis]

Good evening Brian,

My co-mate Axel tried to correct the bug but he don't succeed.

We are sorry.

You can assign this bug to other person.

We were enjoyed to work for you.

Good continuation.

Best regards :)

Alexis Zurawska

Comment 40

[zurawska.alexis]

Good afternoon, 

I began my new simester two weeks ago and I have a course of "Open Source software developpement" which begin today. In this context, I can work in the project that I choosed last simester (this project). So, I have the pleasure to say to you that I will attempt to correct the bug in the context of this course.

Do you want?

Best regards :)

Alexis Zurawska

Comment 41

[Brian Grinstead [:bgrins]]

Sure, you are already assigned.  I think you have enough instruction to begin working on it, have you been able to apply the existing patch and run the browser with the changes?

Comment 42

[zurawska.alexis]

Good morning Brian,

I think I succeded to integrate patch but I hadn't test to run browser with changes because Hacene and Nassim had to work on the bug correction but they hadn't succeded. I will compile code a new time and test in the context of this course.

I'm sorry to answer only now but this week-end, I moved and I haven't internet access during 2 weeks (until Maarch 13 about, maybe less).

Have a nice day.

Best regards :)

Alexis Zurawska

Comment 43

[zurawska.alexis]

Good morning Brian,

With my new co-mate and me attempted to bring modifications in the code but when we update source code of Mozilla and compile a new time, we have errors. My co-mate have error when he execute config file and I have problem when I execute client.mk command. Furthermore, we don't succeed to start our exeutable. We have an error report (crash report). Do you know what's the problem.

Thanks you.

Best regards :)

Alexis Zurawska

Comment 44

[Brian Grinstead [:bgrins]]

(In reply to zurawska.alexis from comment #43)
> With my new co-mate and me attempted to bring modifications in the code but
> when we update source code of Mozilla and compile a new time, we have
> errors. My co-mate have error when he execute config file and I have problem
> when I execute client.mk command. Furthermore, we don't succeed to start our
> exeutable. We have an error report (crash report). Do you know what's the
> problem.

First try building without any other changes from this patch included, just with an updated source tree.  This should work, especially if you had a working build last time you worked on it.  But if you are getting errors even in that case, try asking for help in the #introduction IRC channel.

Comment 45

[zurawska.alexis]

Good evening, 

I studied my problem with my teacher. In fact, I don't succeed to compile my source code because my gcc version is older. I will try to change it and if I don't succeed, I will doing an upgrade of my ubuntu version.

Best reguards :)

Alexis Zurawska

Comment 46

[zurawska.alexis]

Good evening,

I succeeded to change my gcc version and I succeeded to compile my source code. However, I have a problem when I want compile a new time with updates because my terminal indicate that I must create a clobber file. I tried it but compilation stop with others errors. What's the problem?

Thanks :)

Best regards,

Alexis Zurawska

Comment 47

[zurawska.alexis]

Good evening,

I resolved my problem. I had to add a line in my mozconfig file and now all is good.

However, source code compilation is very long (approximately 2 hours). 

It is possible to work and compile unically the source code files on what we work with my co-mate?

If it's possible, how can we do?

Thanks for your answers.

Best regards :)

Alexis Zurawska

Comment 48

[Patrick Brosset <:pbro>]

Glad you figured out the problem and can now start working on a patch, however, I really suggest you ask questions on the mozilla IRC #introduction channel when faced with problems like these. Getting the dev environment right takes some time and there are a lot of people there that can help. There's also a lot of good documentation here https://wiki.mozilla.org/DevTools/Hacking
Can I suggest that you try and refrain from adding too many comments about this in the bug? That makes it harder for other people to read it later (I'm marking a whole lot of comments as obsolete to help).

As Brian said I think, there's a patch attached to the bug already that sort of works, so now that you can compile, you should be able to apply the patch locally, test it, and work from there.

Comment 49

[zurawska.alexis]

Good afternoon, 

Thanks for your informations. I added patch without problem.

I want to connect me to #introduction channel in chatzilla but I don't know in which network I must connect me (moznet, ircnet,...).

Thanks for your answer.

Best regards :)

Alexis Zurawska

P.S: making patch documentation is very good :)

Comment 50

[Patrick Brosset <:pbro>]

(In reply to zurawska.alexis from comment #49)
> I want to connect me to #introduction channel in chatzilla but I don't know
> in which network I must connect me (moznet, ircnet,...).
This should have the information you need: https://wiki.mozilla.org/IRC

Comment 51

[zurawska.alexis]

Good morning,

Thanks :)

I had answers on my problems.

Best regards :)

Alexis Zurawska

Comment 52

[zurawska.alexis]

Good afternoon, 

I have a new problem. Since I integrated the submit patch, inspector of my executable file doesn't print anithing of a web page as he should doing.

So, it is possible to remove patch of my source code version?

Thanks for yurs answers.

Best regards :)

Alexis Zurawska

Comment 53

[Brian Grinstead [:bgrins]]

(In reply to zurawska.alexis from comment #52)
> Good afternoon, 
> 
> I have a new problem. Since I integrated the submit patch, inspector of my
> executable file doesn't print anithing of a web page as he should doing.
> 
> So, it is possible to remove patch of my source code version?

You can use `hg qpop` to remove the current applied patch (see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Using_Mercurial_queues).  Again, the #introduction channel on mozilla's IRC is a good place to get help with getting your development environment up and running.

Comment 54

[zurawska.alexis]

Attachment: copyOuterHTML5.patch

Hello Brian,

I submit to you our correction patch which, as far as we are concerning, is working :).

What do you think about it?

Have a nice day.

Best regards.

Alexis Zurawska

Comment 55

[Brian Grinstead [:bgrins]]

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`

Comment 56

[Patrick Brosset <:pbro>]

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.

Comment 57

[Brian Grinstead [:bgrins]]

(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

Comment 58

[zurawska.alexis]

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

Comment 59

[Brian Grinstead [:bgrins]]

(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.

Comment 60

[zurawska.alexis]

Good evening Brian,

I'm sorry to contact you only today but I haven't succeded to correct this bug. Furthermore, I effectuate a training during three months until end of August. So I haven't time.

So, you can assign this bug to someone else.

It was a pleasure to work with you.

Best regards and good continuation :)

Alexis Zurawska

Comment 61

[Julian Descottes]

Hi Brian,

I would like to work on this one, can you assign it to me ?

Comment 62

[Brian Grinstead [:bgrins]]

(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.

Comment 63

[Julian Descottes]

Attachment: Bug968241.patch

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.

Comment 64

[Brian Grinstead [:bgrins]]

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

Comment 65

[Patrick Brosset <:pbro>]

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 + '"' : '') +
         '>';
},

Comment 66

[Julian Descottes]

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 ?

Comment 67

[Patrick Brosset <:pbro>]

(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)

Comment 68

[Julian Descottes]

Attachment: Bug968241-review.patch

Only attached what changed since my previous patch. Let me know if you want me to upload a patch with a single commit instead.

Comment 69

[Julian Descottes]

Attachment: Bug968241-review.patch

Forgot to remove the duplicate entry in browser.ini in the previous patch.

Comment 70

[Julian Descottes]

Attachment: Bug968241-merged.patch

As discussed, merged the commits (and rebased as well).

Comment 71

[Patrick Brosset <:pbro>]

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.

Comment 72

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfac14009a5f

Comment 73

[Julian Descottes]

Good afternoon Patrick,

Thanks for the review & validation !

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0757f3c684f9

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0757f3c684f9

Comment 77

[Will Bamberg [:wbamberg]]

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!

Comment 78

[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 !