1222737 - Pasting computed rule "text-align>left" (or right) renders "text-alignleft" instead of "text-align: left;"

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Loic]

STR:
1) Open the data URI (testcase)
2) Select the <div> in the Inspector and click on the "Computed" tab
3) Right click and select "Select All" then "Copy"
4) Paste the copied rule in Notepad e.g.


Result:
"text-alignleftwidth: 25px;"
instead of
"text-align: left;width: 25px;"

Comment 1

[Loic]

NB: "text-align: right;" is affected too, not "center" or "justify".

Comment 2

[Loic]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f8a683dfc42&tochange=a2c31dc69ab3

My guess goes to:
Mihai Sucan — Bug 916997 - All lines are joined into one line when copying text from the Web Console; r=past

even if there is:
Michael Ratcliffe — Bug 917863 - Add XUL context menu back into rule and computed views. r=jwalker

Comment 3

[Tom Tromey :tromey]

The bug here is that the selection code just copies the node content directly:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/style-inspector-menu.js#371

The rule view "works around" this by arranging for the text to appear in the form
you'd want to copy.

Cancelling the ni?.  I don't know a good way to say this but Mihai has passed away.

Comment 5

[Patrick Brosset <:pbro>]

The problem seems to be in the way we handle the copy event.
See this function: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#709

Should be a good enough first bug. Make sure you first go through our contribution guide if you're interested in participating:
https://developer.mozilla.org/en-US/docs/Tools/Contributing

Comment 7

[Feng Guo]

(In reply to Patrick Brosset [:pbro] from comment #5)
> The problem seems to be in the way we handle the copy event.
> See this function:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> computed/computed.js#709
> 
> Should be a good enough first bug. Make sure you first go through our
> contribution guide if you're interested in participating:
> https://developer.mozilla.org/en-US/docs/Tools/Contributing

Hi Patrick, I am very new to the community and want to work on this bug. Thanks

Comment 8

[Patrick Brosset <:pbro>]

Thanks for your interest, I'll assign the bug to you now.
As I said in comment 5, your first step is to go through our contribution guide 
https://developer.mozilla.org/en-US/docs/Tools/Contributing
and make sure you are able to build and run firefox locally, make changes to it and reload those changes.

As for fixing this bug in particular, I haven't investigated what exactly is failing, but I suggest using the browser toolbox to debug the code:
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
The browser toolbox allows to use devtools on devtools! So you can debug the devtools code from another devtools debugger.
In the browser toolbox's debugger, you should open the computed.js file and add a breakpoint somewhere in the copySelection function:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#709
and step through the function to investigate what's going wrong.

You can also add console.log() statements in the code and open the browser console (ctrl+shift+J) to see the output if that helps you debug.

Comment 9

[Feng Guo]

Thanks for the detail information, I try to reproduce the bug

The result looks like this:

color: rgb(64, 64, 64);
font-family: "Fira Sans","Open Sans","Helvetica Neue",Arial,Helvetica,sans-serif;
font-size: 13px;
margin-bottom: 20px;
margin-left: 0px;
margin-right: 0px;
margin-top: 0px;
width: 845px;

It looks alright to me?

Comment 10

[Feng Guo]

okay, I see the problem.

Comment 11

[Feng Guo]

Attachment: csscomputedview.propterynames was wrongfully include value for Text-align. fix copy function for text-align

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8742093 [details] [diff] [review]
csscomputedview.propterynames was wrongfully include value for Text-align. fix copy function for text-align

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

::: devtools/client/inspector/computed/computed.js
@@ +704,5 @@
>    },
>  
>    /**
>     * Copy the current selection to the clipboard
> +   *

Please don't insert an empty comment line here.

@@ +709,2 @@
>     */
> +   //Bug 1222737:  text-align copied

No need to add comments with bug numbers when you fix one. The mercurial history log can be used for this instead.

@@ +716,5 @@
>        // Tidy up block headings by moving CSS property names and their
>        // values onto the same line and inserting a colon between them.
>        let textArray = text.split(/[\r\n]+/);
>        let result = "";
> +      let test_str="";

We use camelcase in our styleguide. Please refer to: https://wiki.mozilla.org/DevTools/CodingStandards#Code_style

@@ +722,5 @@
>        if (textArray.length > 1) {
>          for (let prop of textArray) {
> +          //
> +          //fix for text-align
> +          if (CssComputedView.propertyNames.indexOf(prop) !== -1 && test_str !="text-align") {

Here you are attempting to fix the bug only, not its root cause.
Which means that another, similar, bug can come back later.

Instead, you should find out what is the root cause for this bug and fix this instead.

After looking at it a bit, I found out that the problem is this:

  CssComputedView.propertyNames.indexOf(prop) !== -1

Here we search for prop in a list of property names. If we find it, then we assume it must be a name. If we don't, then we assume it's a value.
And then we just construct the string to be copied with: name : value ;

The problem is that the word "left" in css can be both a name and a value. So this whole logic is flawed and will break in other cases too.

I am not 100% sure of this, but I think it is safe to assume here that textArray will always contain name, then value, then name, then value, etc...
So I would not use CssComputedView.propertyNames here at all and instead just loop through the textArray and build the output string as I go, considering even items to be names and odd items to be values.

Comment 15

[Patrick Brosset <:pbro>]

Hi Feng, are you still interested in fixing this bug?

Just to re-iterate on my previous review comment, we need to fix the root cause of the problem. As you can see, bug 1201750 was closed as a duplicate of this and, even though it has different steps and results, the root cause is the same.

So, in copySelection, you get the text that is currently selected by the user, and you need to format this text into something that looks like CSS so the user can use it more easily.

Turns out this text is already split on different lines, so that makes our life easier.
For instance, if you just split this text like so: 
let textArray = text.split(/[\r\n]+/);

which is what copySelection already does, you get an array of [name, value, name, value, name, value, ...]

The rest of copySelection has a bug because it tries to guess which item of the array is a value and which is a name by comparing to a list, but as I said comment 12 there are cases where this will fail.

If instead you'd simply iterate over the array like so:

let result = "";
for (var i = 0; i < textArray .length; i += 2) {
  result += textArray [i] + ": " + lines[i + 1] + ";\n";
}

I think that would work better.

Now, we also have to think about the case where you expand a property. As you can see, in the computed-view, some properties can be expanded to list the css rules where the values comes from. These will need to be formatted differently. But for now, if you ignore that and fix the first problem, that would be a great start.

Comment 16

[Patrick Brosset <:pbro>]

Ah, but of course this only works if the text selection starts at a name. If you select text from a value this won't work.
So, what you can do, is use the Selection API: https://developer.mozilla.org/en-US/docs/Web/API/Selection#Properties to detect where the selection starts. You can check the anchorNode and focusNode, determine which comes first in the DOM so that you know which is the first selected element, and based on its className, you can know if its a name or a value. Based on this you can adapt the loop I mentioned in comment 15 to be correct.

Comment 17

[arni2033]

Comment 16 (15) wouldn't work if there's multiple selection. And that wouldn't work if some text from
"best matches" is selected. This bug will take you forever and will probably use something better than
messing with getSelection().
Therefore existance of this bug shouldn't be used as an excuse for keeping bug 1201750 which has an
easiest fix that would make Ctrl+A consistent with text selection with mouse (click and drag)

Comment 18

[arni2033]

* for keeping bug 1201750 which is more "global" and noticeable (happens with all rules)

Comment 19

[Feng Guo]

Sorry, I was on my summer vacation and I will finish it.
(In reply to Patrick Brosset <:pbro> from comment #16)
> Ah, but of course this only works if the text selection starts at a name. If
> you select text from a value this won't work.
> So, what you can do, is use the Selection API:
> https://developer.mozilla.org/en-US/docs/Web/API/Selection#Properties to
> detect where the selection starts. You can check the anchorNode and
> focusNode, determine which comes first in the DOM so that you know which is
> the first selected element, and based on its className, you can know if its
> a name or a value. Based on this you can adapt the loop I mentioned in
> comment 15 to be correct.

Comment 20

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 21

[Patrick Brosset <:pbro>]

No activity in the past 5 months. Returning to unassigned in case someone wants to pick up where we left off.

Comment 22

[rambo1177]

Hey, I'm new to the community and thought I'd work on this. Correct me if I'm wrong, but since the copying is actually done to produce immediately usable css, the parent properties seem rather redundant. From what I gather this problems seems to be only with parent properties.

Comment 23

[Tom Tromey :tromey]

(In reply to rambo1177 from comment #22)
> Hey, I'm new to the community and thought I'd work on this. Correct me if
> I'm wrong, but since the copying is actually done to produce immediately
> usable css, the parent properties seem rather redundant. From what I gather
> this problems seems to be only with parent properties.

I think the original report describes the bug pretty well.
Basically, copying in the computed view produces something that isn't valid CSS; but
that would be more useful if it were valid CSS.  I don't think parent properties
enter the picture, really.

Comment 24

[rambo1177]

(In reply to Tom Tromey :tromey from comment #23)
> I think the original report describes the bug pretty well.
> Basically, copying in the computed view produces something that isn't valid
> CSS; but
> that would be more useful if it were valid CSS.  I don't think parent
> properties
> enter the picture, really.

Fair enough. And like you mentioned before this has more to do with the formatting of the initial arrangement. 
Also I thought it'd be easier if we could just iterate through the selected nodes, something that isn't readily available in the Selection API or I've been looking in the wrong place. Is there anywhere you could steer me to ?

Comment 25

[Patrick Brosset <:pbro>]

I'm not sure if this feature is worth it to be honest. As said in comment 17, if there are multiple selection, or if one of the properties is expanded (to show the best match rule), or if the text selection starts in the middle of words, then it will be really hard to copy a a list of <property>: <value>; lines to the clipboard.
I'm wondering if we shouldn't just copy exactly what is selected, instead of trying to reformat it. It seems like trying to format like now will always have weird edge cases.

Comment 26

[hanbin.chang]

Hi, I am new to the community and I would like to work on this as my first bug.
Thank you,

Comment 27

[Patrick Brosset <:pbro>]

Hi, thanks for wanting to work on this!
Have you read our contribution guide yet: https://developer.mozilla.org/en-US/docs/Tools/Contributing ?
This will be useful to know how to get the code, make a change and submit that change.

In terms of what to actually fix here, I had a chat with :jdescottes, :zer0 and :tromey this morning and we ended up deciding the following:

There are edge cases that make the feature buggy currently:
- the test case in comment 0 shows the original bug
- also, if you expand properties in the computed panel and then copy, the result won't be a nicely formatted list of property:value;
- finally, if you have multiple text selections enabled on your computer and you do select multiple regions of text, then the result is going to be weird too.

So, we decided that the auto-formatting of what you copied to property:value; pairs wasn't worth doing in the way we do it now. Right now, we try to do this automatically somehow, but the logic we use breaks down in the 3 cases I listed above.
The agreement we ended up with was that the DOM of the computed panel should contain colon ":", semi-colon ";" and new-lines "\n" so that copying the text would just work, without us doing anything special in javascript.
Now, these characters could be hidden so you don't actually see them, but copying the text would copy them too.

This way, the panel would not need to have any special javascript code that reformats everything, the formatting would already be contained in the text you're copying.
If you add multiple selections, or expanded properties, then you'd just copy what you have selected, instead of trying to reformat that too.

If you see something, select it, copy it, you expect it to look the same once pasted.

Does that make sense?

The re-formatting logic we would like to remove is here: http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/devtools/client/inspector/computed/computed.js#734-775

Comment 28

[Santiago Paez [:tiago]]

Hi Patrick!

I'm new around here and I would like to work on this bug :)

Before starting I have some questions,

I can re-format the logic of copySelection to reflect the new behavior, but after this, if we copy the computed rules and paste them somewhere, they won't be nicely css-formated. I understand your idea of adding colon ":", semi-colon ";" and new-lines "\n" to the computed panel. I just can't easily find where to do those changes. Any idea where that could be?

Thanks!

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Santiago Paez [:tiago] from comment #28)
> I understand your idea of adding colon ":",
> semi-colon ";" and new-lines "\n" to the computed panel. I just can't easily
> find where to do those changes. Any idea where that could be?
Sure! And thanks for working on this.
I believe you want to look at the buildMain function inside devtools\client\inspector\computed\computed.js
This is where each and every property displayed in the panel gets created.
So if you insert something like <span>:</span> and <span>;</span> after the name and after the value in this function, they will be part of the DOM.
Now we need to find a way for these to remain hidden but still copied to the clipboard when the text is selected and the user hits ctrl+C or uses the context menu.

Comment 30

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: added colon and semi-colons to the dom and formatted line-breaks in copySelection

Hi,

What's the status on this?

I've attached a WIP patch that inserts <span> elements for ":" and ";" into the DOM, and formats the line breaks appropriately in copySelection.

Next step is just to hide visually the spans via css.

Does this approach sound reasonable?

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8860745 [details] [diff] [review]
added colon and semi-colons to the dom and formatted line-breaks in copySelection

Sorry I can't review this patch, it seems like it is invalid. It looks like it's just a copy/paste of the code, rather than a diff generated from HG or GIT.
This doc might be useful to you: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 32

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

Review commit: https://reviewboard.mozilla.org/r/132950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132950/

Comment 33

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #31)
> Sorry I can't review this patch, it seems like it is invalid. It looks like
> it's just a copy/paste of the code, rather than a diff generated from HG or
> GIT.

Very sorry for this, I made the same mistake in another bug - looks like i was relying on outdated docs. I've send it through mozreview.

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

https://reviewboard.mozilla.org/r/132950/#review136708

Great start. Thanks for working on this.
There's a problem when you expand one of the properties in the panel and then try to copy the whole text, we miss a line break in there.
Also, I have a few questions below:

::: devtools/client/inspector/computed/computed.js:737
(Diff revision 1)
>        let text = win.getSelection().toString().trim();
> -      // isPropertyPresent is set when a property name is spotted and
> -      // we assume that the next line will be a property value.
> -      let isPropertyPresent = false;
> -      // Tidy up block headings by moving CSS property names and their
> -      // values onto the same line and inserting a colon between them.
>        let textArray = text.split(/[\r\n]+/);
> -      let result = "";
>  
> -      // Parse text array to output string.
> -      if (textArray.length > 1) {
> -        for (let prop of textArray) {
> -          if (CssComputedView.propertyNames.indexOf(prop) !== -1) {
> -            // Property name found so setting isPropertyPresent to true
> -            isPropertyPresent = true;
> +      // Strip extra line breaks and format them correctly
> +      let result = textArray.reduce(function (acc, curr, i) {
> +        return ((i + 1) % 3 == 0 && i < textArray.length - 1)
> +          ? acc + curr + "\n"
> +          : acc + curr;
> +      }, "");

Do we need to do this? The reason we chose to add the : ; and \n to the markup directly was so that we wouldn't have to deal with parsing the text being copied.
I think we want to just put in the clipboard whatever is currently selected.

::: devtools/client/inspector/computed/computed.js:1006
(Diff revision 1)
>      this.onFocus = () => this.element.focus();
>      this.nameNode.addEventListener("click", this.onFocus);
> +
> +    // Build the style name ":" separator
> +    let nameSeparator = doc.createElementNS(HTML_NS, "span");
> +    nameSeparator.setAttribute("class", "visually-hidden");

This class does not exist yet, so the : and ; characters are visible in the panel now.
How do you plan to hide them while still making them copy-able?

Comment 35

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

https://reviewboard.mozilla.org/r/132950/#review136708

> Do we need to do this? The reason we chose to add the : ; and \n to the markup directly was so that we wouldn't have to deal with parsing the text being copied.
> I think we want to just put in the clipboard whatever is currently selected.

Yes, it makes sense and I totally get that. The problem is if you don't do this in copySelection, you end up with extra line breaks. You can debug and see that it will look like:

text-align:
left
;
width:
25px
;

This is due because of the way Firefox is implementing getSelection() for multiple block elements. Even if you style them "inline" it will add an extra newline "\n" in between. I haven't looked at the code, but this is easy to replicate:
1) Open in the browser: 
data:text/html;charset=utf-8,<div style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">this is<%2Fdiv><div style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">on the same line<%2Fdiv>
2) Select all text
3) Enter in the console window.getSelection().toString();

When you compare the console outputs on Chrome and on Firefox, you will see no "\n" added on Chrome.

Also:

data:text/html;charset=utf-8,<span style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">this is<%2Fspan><span style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">on the same line<%2Fspan>"

doesn't add any extra "\n", which is perfectly fine.


The spec is pretty vague about it: https://www.w3.org/TR/selection-api/#idl-def-window-getselection() , but I'd say it is likely to be a bug. Should I report this?

So, to resume, we could still be able to skip that extra step in copySelection, but it would involve restructuring the html and replace the blocks elements we have right now with inline elements.

Comment 36

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

https://reviewboard.mozilla.org/r/132950/#review136708

> This class does not exist yet, so the : and ; characters are visible in the panel now.
> How do you plan to hide them while still making them copy-able?

I was thinking to do it by adding a new class in in computed.css, either

.visually-hidden { opacity: 0; }

which should definitely work, or maybe something a bit more fancy like :

.visually-hidden { display: inline-block; text-indent: -9999px; }

Comment 37

[Patrick Brosset <:pbro>]

(In reply to Matt R from comment #35)
> Comment on attachment 8860939 [details]
> Bug 1222737 - Fix missing colons and semi-colons in renders of pasted
> computed rules;
> 
> https://reviewboard.mozilla.org/r/132950/#review136708
> 
> > Do we need to do this? The reason we chose to add the : ; and \n to the markup directly was so that we wouldn't have to deal with parsing the text being copied.
> > I think we want to just put in the clipboard whatever is currently selected.
> 
> Yes, it makes sense and I totally get that. The problem is if you don't do
> this in copySelection, you end up with extra line breaks. You can debug and
> see that it will look like:
> 
> text-align:
> left
> ;
> width:
> 25px
> ;
> 
> This is due because of the way Firefox is implementing getSelection() for
> multiple block elements. Even if you style them "inline" it will add an
> extra newline "\n" in between. I haven't looked at the code, but this is
> easy to replicate:
> 1) Open in the browser: 
> data:text/html;charset=utf-8,<div
> style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">this is<%2Fdiv><div
> style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">on the same line<%2Fdiv>
> 2) Select all text
> 3) Enter in the console window.getSelection().toString();
> 
> When you compare the console outputs on Chrome and on Firefox, you will see
> no "\n" added on Chrome.
> 
> Also:
> 
> data:text/html;charset=utf-8,<span
> style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">this is<%2Fspan><span
> style%3D"text-align%3Aleft%3Bdisplay%3Ainline%3B">on the same line<%2Fspan>"
> 
> doesn't add any extra "\n", which is perfectly fine.
I wasn't aware of this. Thanks!

> The spec is pretty vague about it:
> https://www.w3.org/TR/selection-api/#idl-def-window-getselection() , but I'd
> say it is likely to be a bug. Should I report this?
I think you should. This is probably the kind of things that has been discussed in the past, but filing a bug would be a good way to know what the state of the discussion was and what is the "right" way.

> So, to resume, we could still be able to skip that extra step in
> copySelection, but it would involve restructuring the html and replace the
> blocks elements we have right now with inline elements.
I think I'd prefer this approach, this way the JS code can be really simple, and there doesn't need to be any magic for cases when you have things like multiple selections, or you expanded one or several properties, etc...
The code changes to the HTML seem minor enough that this would be worth it.

(In reply to Matt R from comment #36)
> > This class does not exist yet, so the : and ; characters are visible in the panel now.
> > How do you plan to hide them while still making them copy-able?
> 
> I was thinking to do it by adding a new class in in computed.css, either
> 
> .visually-hidden { opacity: 0; }
> 
> which should definitely work, or maybe something a bit more fancy like :
> 
> .visually-hidden { display: inline-block; text-indent: -9999px; }
Any of these 2 solutions sound fine to me.

Comment 38

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #37)

> I think you should. This is probably the kind of things that has been
> discussed in the past, but filing a bug would be a good way to know what the
> state of the discussion was and what is the "right" way.

Will do then!

> I think I'd prefer this approach, this way the JS code can be really simple,
> and there doesn't need to be any magic for cases when you have things like
> multiple selections, or you expanded one or several properties, etc...
> The code changes to the HTML seem minor enough that this would be worth it.

Agree with this

Thanks for the insight! I will post an updated patch tonight or tomorrow

Comment 39

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132950/diff/1-2/

Comment 40

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #34)

> There's a problem when you expand one of the properties in the panel and
> then try to copy the whole text, we miss a line break in there.

If we expand and then select all + copy, the pasted text for a row looks like:

text-align:left;

element
this.styleleft

Do you mean we should just add a line break between "this.style" and "left" ? 
If yes, we can fix it the same way by modifying the html

Comment 41

[Patrick Brosset <:pbro>]

(In reply to Matt R from comment #40)
> (In reply to Patrick Brosset <:pbro> from comment #34)
> 
> > There's a problem when you expand one of the properties in the panel and
> > then try to copy the whole text, we miss a line break in there.
> 
> If we expand and then select all + copy, the pasted text for a row looks
> like:
> 
> text-align:left;
> 
> element
> this.styleleft
> 
> Do you mean we should just add a line break between "this.style" and "left"
> ? 
> If yes, we can fix it the same way by modifying the html
Yes that would be great. Sorry my comment wasn't clear.

Comment 42

[Patrick Brosset <:pbro>]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

https://reviewboard.mozilla.org/r/132950/#review137216

This looks great, but I was unable to import the commit locally and test it. I think there must have been a problem when you pushed to mozreview, because when I look at the diff in mozreview, some of the changes I know you made are not being highlighted. And when I try to import the commit, it won't apply without an error.
Could you look into please? Maybe pull the latest, and rebase your whole commit on top of it?

::: commit-message-7513b:1
(Diff revision 2)
> +Bug 1222737 - removed extra logic in copySelection, changed <div> elements to <span>; r?pbro

It would be better to keep this first line of the commit message to explain what and why you did. And then you can explain more details (like replacing divs with spans) in subsequent lines.

Suggestion:

```
Bug 1222737 - Handling selection copy logic in HTML directly; r?pbro

Added : and ; characters to the DOM directly, visually
hidden so they won't appear byt will be part of the copied
text.
Changed divs to spans to make sure line breaks exist too.
And simplified the logic in copySelection as a result.
```

Comment 43

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132950/diff/2-3/

Comment 44

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860939 [details]
Bug 1222737 - Added extra line break when expanding properties in the panel and selecting all;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132950/diff/3-4/

Comment 45

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1222737 - Fix missing colons and semi-colons in renders of pasted computed rules;

Review commit: https://reviewboard.mozilla.org/r/135484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135484/

Comment 46

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1222737 - Add colons and semi-colons in copied computed properties;

Added : and ; characters to the DOM directly, visually hidden so they won't appear but will be part of the copied text.
Changed divs to spans to make sure we don't have extra line breaks.
And simplified the logic in copySelection as a result.

Review commit: https://reviewboard.mozilla.org/r/135486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135486/

Comment 47

[Patrick Brosset <:pbro>]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

As discussed on IRC:

The code changes look good to me. But I think we still need some clean up done  to your commits: 
- I think your 2 first commits should be squashed into one.
- The last commit shouldn't be there at all.

I can also do that for you before landing the change if you want.

More importantly, we need to check if tests still pass, and possibly write a new one. In particular, you should try to run this test locally: devtools\client\inspector\computed\test\browser_computed_select-and-copy-styles.js
If it still passes, that's a good start! Maybe it needs to be adapted.

You should also add a new case to this test that makes sure the problem described in comment 0 is actually fixed.

Comment 48

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1222737 - Modified tests for computed properties;

Added extra space to the : separator to match the old behavior and reduce failed tests.
Refactored the test file slightly for better flexibility regarding multiple test cases and tests.
Added new test case to assert the problem described and to account for expanded properties

Review commit: https://reviewboard.mozilla.org/r/136786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136786/

Comment 49

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #47)
> You should also add a new case to this test that makes sure the problem
> described in comment 0 is actually fixed.

- As discussed on IRC, modified computed.js to include a space after the ":" separator
- Reorganised the file slightly for more flexibility regarding multiple test cases
- Added a new test case to assert against the problem described
- Accounted for expanded properties in that test case

Comment 50

[Patrick Brosset <:pbro>]

Comment on attachment 8865123 [details]
Bug 1222737 - Modified tests for computed properties;

https://reviewboard.mozilla.org/r/136786/#review141548

This test change looks good to me! I would however prefer if the 2 test cases were in 2 different files. That would make each test case easier to read and debug.

Also, there are still 4 separate commits on mozreview which I think should be merged as I said last time. One thing to take into consideration is that mozreview shows commit separately, so when you address a review comment and push a new commit to mozreview, then I see it separately. There's no way for me to see the complete code changes.
This is very different than GitHub pull requests for instance. This is why it's better to ammend your commits when you just make small adjustment to them and push again. Mozreview is good at detecting that a commit was updated and it even has a interdiff feature if I'm interested in seeing what you changed last only.
Now, that doesn't mean we can't have several commits. In fact it's good to have several of them when they're split in logical chunks. Maybe one part for the code, one for the test. Or sometimes big changes can be done in several code change parts.
But here, on this particular review, the 4 commits are only representative of the various fixes you made through time.

I've merged the commits locally, and split the test changes in a 2nd commit, and extracted your new test case to a new test file.
So I've done no code changes, just moved yours around a little bit so it's easier to review, and so that the mercurial repo history is easier to read too.

I'll push those here for you.

Comment 51

[Patrick Brosset <:pbro>]

Attachment: Bug 1222737 - Add colons and semi-colons in copied computed properties;

Added : and ; characters to the DOM directly, visually hidden so they
won't appear but will be part of the copied text.
Changed divs to spans to make sure we don't have extra line breaks.
And simplified the logic in copySelection as a result.

Review commit: https://reviewboard.mozilla.org/r/138388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138388/

Comment 52

[Patrick Brosset <:pbro>]

Attachment: Bug 1222737 - Tests for the computed-view copy feature;

Review commit: https://reviewboard.mozilla.org/r/138390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138390/

Comment 53

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #50)
> This test change looks good to me! I would however prefer if the 2 test
> cases were in 2 different files. That would make each test case easier to
> read and debug.
> 

Ok, I agree that it probably makes sense to have it in a separated file.

> One thing to take into consideration is that
> mozreview shows commit separately, so when you address a review comment and
> push a new commit to mozreview, then I see it separately. There's no way for
> me to see the complete code changes.

Oh I see my mistake now. Sorry for not understanding this earlier.
Just to be sure I have it correctly, I should just do "git commit --amend && git mozreview push" when doing minor fixes/updates, and "git commit && git mozreview push" (which creates a new commit) when it is suitable to have logically separated commits?

> I've merged the commits locally, and split the test changes in a 2nd commit,
> and extracted your new test case to a new test file.

Thanks very much for cleaning the commit history! I'm sorry I got a bit confused by how the correct process should be in mozreview, hopefully it will go smoother in next patches :)

> So I've done no code changes, just moved yours around a little bit so it's
> easier to review, and so that the mercurial repo history is easier to read
> too.
> 
> I'll push those here for you.

Thank you again for this!
Is there anything left you'd want me to do?

Comment 54

[Patrick Brosset <:pbro>]

(In reply to Matt R from comment #53)
> Oh I see my mistake now. Sorry for not understanding this earlier.
No problem! Mozreview is special, I should have explained this part to you.
> Just to be sure I have it correctly, I should just do "git commit --amend &&
> git mozreview push" when doing minor fixes/updates, and "git commit && git
> mozreview push" (which creates a new commit) when it is suitable to have
> logically separated commits?
I use Mercurial myself, so I'm not totally sure about the exact commands to be run, but yes, that's the idea.

> hopefully it will go smoother in next patches :)
I'm sure it will! And again, don't worry, cleaning up commits is really not a big deal, I'm very thankful for the code you wrote, that's the most important part.

> Is there anything left you'd want me to do?
I haven't run the tests locally. Could you import the 2 new commits on your own checkout, and run the tests to make sure they pass? If they do, the next step will be to push these commits to the CI environment and check that everything is fine there. Finally, we can land these commits to the mozilla-central repository.

Let me know if you need help running the tests. I suggest using the following mach command for this:
./mach test devtools/client/inspector/computed/test  --> this runs all computed-view integration tests
./mach test devtools/client/inspector/computed/test/browser_computed_select-and-copy-styles-01/2.js  --> to run just one test

Comment 55

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8866776 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

https://reviewboard.mozilla.org/r/138388/#review142296

::: devtools/client/inspector/computed/computed.js:1146
(Diff revision 1)
>        let shortcuts = new KeyShortcuts({
>          window: this.tree.styleWindow,
>          target: link
>        });
>        shortcuts.on("Return", () => selector.openStyleEditor());
>  

Here I believe we just miss some slights amends from the old commits.

- Splitting 'status' into 2 inline <div> to have that extra line-break in the formatting.
- append the 'inline' class

So that block becomes eventually:

let status = createChild(p, "span", {
  dir: "ltr",
  class: "rule-text theme-fg-color3 " + selector.statusClass,
  title: selector.statusText
});

let keyDiv = createChild(status, "div", {
  class: "inline",
  textContent: selector.sourceText
});

let valueDiv = createChild(status, "div", {
  class: "inline other-property-value theme-fg-color1"
});

valueDiv.appendChild(selectot.outputFragment);

::: devtools/client/themes/computed.css:135
(Diff revision 1)
>  }
>  
>  .theme-firebug .property-view {
>    border-bottom: 1px solid rgba(0, 0, 0, 0.1);
>  }
>  

I believe we just miss this line from the old commits:

.inline {
  display: inline;
}

Comment 56

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #54)

> I haven't run the tests locally. Could you import the 2 new commits on your
> own checkout, and run the tests to make sure they pass?

The test for devtools/client/inspector/computed/test/browser_computed_select-and-copy-styles-02.js was failing, but it is just because some tiny bits were missing from the old commits.

I've added some comments, the tests are all passing for me with these added.

Comment 57

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135486/diff/1-2/

Comment 58

[Patrick Brosset <:pbro>]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

https://reviewboard.mozilla.org/r/135486/#review143922

Looking good! Great to hear that tests now pass.
I just have one final comment about the `inline` class.
Then we should be good to go.

::: devtools/client/themes/computed.css:136
(Diff revision 2)
> +.inline {
> +  display: inline;
> +}

Creating DIVs and then adding a class named 'inline' on them, that does 'display: inline' is a little strange.
Future readers of this code will ask themselves why we didn't use an inline element to start with, like a span.

Plus, using a class named after the styles it has is usually frowned upon. Better use a name that says what the class does, something that describes what the element is rather than what it looks like.

What about removing the 'inline' class altogether and instead have a CSS rule like:

```css
.rule-text > div {
  /* We need DIVs inside a rule-text to be inline
     because ... blah blah ... */
  display: inline;
}
```

Comment 59

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

https://reviewboard.mozilla.org/r/135486/#review143966

::: devtools/client/themes/computed.css:136
(Diff revision 2)
> +.inline {
> +  display: inline;
> +}

I agree that the 'inline' name should be changed, I was a bit out of inspiration, what we really do here is just working around Bug 1360238

Your idea of using a 'non-direct' selector does make sense, as we wouldn't have any real functionality behind a specific class. But I'm concerned this behavior would be a bit too much 'behind the scenes' for a subtle bug like this?

I feel someone might encounter a scenario where they don't necessarily need <div>s to behave like this. So I'd rather be more explicit even if the class itself lacks functionality. After all it is a workaround and will be removed eventually.

Maybe just:
```css
.fix-get-selection {
  display: inline;
}
```

(I'm terrible at naming, so maybe not exactly that, but you get the idea)

What do you think?

Comment 60

[Patrick Brosset <:pbro>]

(In reply to Matt R :stylizit from comment #59)
> Comment on attachment 8863712 [details]
> Bug 1222737 - Add colons and semi-colons in copied computed properties;
> 
> https://reviewboard.mozilla.org/r/135486/#review143966
> 
> ::: devtools/client/themes/computed.css:136
> (Diff revision 2)
> > +.inline {
> > +  display: inline;
> > +}
> 
> I agree that the 'inline' name should be changed, I was a bit out of
> inspiration, what we really do here is just working around Bug 1360238
> 
> Your idea of using a 'non-direct' selector does make sense, as we wouldn't
> have any real functionality behind a specific class. But I'm concerned this
> behavior would be a bit too much 'behind the scenes' for a subtle bug like
> this?
> 
> I feel someone might encounter a scenario where they don't necessarily need
> <div>s to behave like this. So I'd rather be more explicit even if the class
> itself lacks functionality. After all it is a workaround and will be removed
> eventually.
> 
> Maybe just:
> ```css
> .fix-get-selection {
>   display: inline;
> }
> ```
> 
> (I'm terrible at naming, so maybe not exactly that, but you get the idea)
> 
> What do you think?
Ok, but please add a comment above this CSS rule that mentions bug 1360238 and why we do this and that we should remove this when the bug is fixed.

Comment 61

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #60)

> Ok, but please add a comment above this CSS rule that mentions bug 1360238
> and why we do this and that we should remove this when the bug is fixed.

Yes, absolutely agree with that, will do.

Comment 62

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135486/diff/2-3/

Comment 63

[Patrick Brosset <:pbro>]

Comment on attachment 8863712 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

https://reviewboard.mozilla.org/r/135486/#review147534

Comment 64

[Patrick Brosset <:pbro>]

Comment on attachment 8866777 [details]
Bug 1222737 - Tests for the computed-view copy feature;

https://reviewboard.mozilla.org/r/138390/#review147536

Comment 65

[Patrick Brosset <:pbro>]

Try push URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf734dea93fe91b6cb41b295ae97f01a7f807a5

Comment 66

[Matt R [:stylizit] (slow, away until Sep)]

It seems that the bustage on the try results occur because the revision sent to treeherder is not the latest one:
https://hg.mozilla.org/try/rev/98701ada3272043c3ffb4a6547255cf3f40c4c51 doesn't include the latest fix.

I think it should be this one instead: https://reviewboard-hg.mozilla.org/gecko/raw-rev/1b22963f4c36

I feel it might be my fault for just pulling that change and pushing it...

On another note the other "oranges" we might have to fix - because we change <span>s to <div>s some other test suites probably need updating as well.

Comment 67

[Patrick Brosset <:pbro>]

Thanks Matt. I'll spend some time on this. I will push another couple of commits to TRY soon.

Comment 68

[Patrick Brosset <:pbro>]

Hopefully the first commit is now the right one. Also I've fixed the test commit. Let's see ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f932d0cc8d6ab48bd7b3ae9dfc325a023c9560c

Comment 69

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14e6a804858246986daf00242d07994cdf8bb74

Comment 70

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Patrick Brosset <:pbro> from comment #69)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b14e6a804858246986daf00242d07994cdf8bb74

Yes, try results seem good now! Thanks for updating the patch

Comment 71

[Patrick Brosset <:pbro>]

Comment on attachment 8866776 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138388/diff/1-2/

Comment 72

[Patrick Brosset <:pbro>]

Comment on attachment 8866777 [details]
Bug 1222737 - Tests for the computed-view copy feature;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138390/diff/1-2/

Comment 73

[Patrick Brosset <:pbro>]

Comment on attachment 8866776 [details]
Bug 1222737 - Add colons and semi-colons in copied computed properties;

https://reviewboard.mozilla.org/r/138388/#review147898

Carrying R+ over

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12d05f45e5bd
Add colons and semi-colons in copied computed properties; r=pbro
https://hg.mozilla.org/integration/autoland/rev/09e9ea98152d
Tests for the computed-view copy feature; r=pbro

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12d05f45e5bd
https://hg.mozilla.org/mozilla-central/rev/09e9ea98152d