1393609 - Styled console logs with unit-less line-height default to px unit

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[:Harald Kirschner :digitarald]

Visible on http://bbc.com/news , reduced test cases in bug URL.

STR:

console.log('%cline-height: 1.5', 'line-height: 1.5; background-color: red; color: white')

Expected:

White letters with red background on a 1.5em line

Actual:

Red bar with white letters flowing over the top. Inspecting the log shows that the inline style is set to line-height: 1.5px

Comment 1

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

This is only affecting the new frontend.
It seems like the server returns the correct style (i.e. line-height: 1.5), and React is adding the "px" at the end.

See https://codepen.io/nchevobbe/pen/QMVLBV?editors=0010

We could get rid of that if we'd transform the "line-height" into "lineHeigth", but I'm not sure it's worth it.
We should see if it's still an issue in React 16

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)
> This is only affecting the new frontend.
> It seems like the server returns the correct style (i.e. line-height: 1.5),
> and React is adding the "px" at the end.
> 
> See https://codepen.io/nchevobbe/pen/QMVLBV?editors=0010
> 
> We could get rid of that if we'd transform the "line-height" into
> "lineHeigth", but I'm not sure it's worth it.
> We should see if it's still an issue in React 16

If it can be fixed by switching to `lineHeight` I'd say let's go with that

Comment 3

[Brian Grinstead [:bgrins]]

I guess that the thing to do here is convert 'line-height' into 'lineHeight' in cleanupStyle before the Object.assign call: https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/GripMessageBody.js#144

It can be tested using the steps in Comment 0 to confirm the fix

Comment 4

[ewhite7]

can i give this bug a try for my open source course?

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #4)
> can i give this bug a try for my open source course?

Sure, i'll assign the bug to you. 
In the meantime you may want to have a look at http://docs.firefox-dev.tools/getting-started/ to clone the firefox repo and build it (make sure to user artifact build, it's faster).
Don't hesitate to ask any question, here or on Slack https://devtools-html-slack.herokuapp.com/

Comment 6

[ewhite7]

thanks. i already have the build

Comment 7

[ewhite7]

i just need the instructions on how to get started

Comment 8

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #7)
> i just need the instructions on how to get started

come in slack so we can chat

Comment 9

[ewhite7]

do i need an invite for slack? or can you send me the link

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

You can use the link from Comment 5

Comment 11

[ewhite7]

because the browser is doing the css parsing is it possible to use  some kind of reggex to edit the line-height before return object.assign

Comment 12

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #11)
> because the browser is doing the css parsing is it possible to use  some
> kind of reggex to edit the line-height before return object.assign

You can have a look at a talk from Mike Taylor where he explains the algorithm to transform css-property to JS equivalent : https://vimeo.com/227704821#t=1290s

If we go this way, we need to check if the performance are okay.
Or, we could have a white list dictionary of properties we want to modify.
For now, it would be 

```js
const cssDictionary : {
  "line-height": "lineHeight"
};
```

And then check each property to see if they're in the dictionary.

Comment 13

[ewhite7]

i was thinking something like this, and it actually works, but i will take a look at the video

function cleanupStyle(userProvidedStyle, createElement) {

  let dummy = createElement("div");
  dummy.style = userProvidedStyle;

  var dums = [...dummy.style]
    .filter(name => { //fat Arrow function
      return allowedStylesRegex.test(name)
        && !forbiddenValuesRegexs.some(regex => regex.test(dummy.style[name]));
    })
    .reduce((object, name) => {
      return Object.assign({
        [name]: dummy.style[name]
      }, object);
    }, {});

    return cleanupStyleTemp(dums);
}

function cleanupStyleTemp(args_){
  args_.lineheight = 1.5
  delete args_['line-height']
  return args_;
}

Comment 14

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #13)
> i was thinking something like this, and it actually works, but i will take a
> look at the video
> 
> function cleanupStyle(userProvidedStyle, createElement) {
> 
>   let dummy = createElement("div");
>   dummy.style = userProvidedStyle;
> 
>   var dums = [...dummy.style]
>     .filter(name => { //fat Arrow function
>       return allowedStylesRegex.test(name)
>         && !forbiddenValuesRegexs.some(regex =>
> regex.test(dummy.style[name]));
>     })
>     .reduce((object, name) => {
>       return Object.assign({
>         [name]: dummy.style[name]
>       }, object);
>     }, {});
> 
>     return cleanupStyleTemp(dums);
> }
> 
> function cleanupStyleTemp(args_){
>   args_.lineheight = 1.5
>   delete args_['line-height']
>   return args_;
> }

Hello ewhite7,
I'm not sure what the snippet you posted here is supposed to do ? You assign aa 1.5 lineheight to args and delete line-height, in a dedicated function.

The thing is, we may not have a line-height object, and if we do, it might not be 1.5.
Since we are already building the object in the reduce call, we could have something like : 


```js
const cssToIdlDictionnary = {
  "line-height": "lineHeight"
};

return [...dummy.style]
    .filter(name => {
      return allowedStylesRegex.test(name)
        && !forbiddenValuesRegexs.some(regex => regex.test(dummy.style[name]));
    })
    .reduce((object, name) => {
      return Object.assign({
        [cssToIdlDictionnary[name] || name]: dummy.style[name]
      }, object);
    }, {});
```

I think that would work well, and is still legible

Comment 15

[ewhite7]

wow thanks man that did the trick. sorry for taking so long, but i am new to JavaScript. how can i submit for testing?

Comment 16

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

So, what you can do is to add a line-height: 1.5 at the end of this string : https://searchfox.org/mozilla-central/rev/d99de806665d227ad71262933800ef6b069b230a/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js#110

This define a stub, i.e. the JSON object we get from the server in response of some commands.
You can see the json object here https://searchfox.org/mozilla-central/rev/d99de806665d227ad71262933800ef6b069b230a/devtools/client/webconsole/new-console-output/test/fixtures/stubs/consoleApi.js#2335-2359.

Now, because we modified the expression to be evaluated, we need to run a command to update the stub.
You can do this by doing `./mach run devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js` in your terminal.

You should then see that the consoleApi.js file was modified.

Now we have a proper stub with the case we're interested in, let's check how it is rendered in a test.
Luckily, we already have a test for styled logs : https://searchfox.org/mozilla-central/rev/d99de806665d227ad71262933800ef6b069b230a/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js#45

So after those lines : https://searchfox.org/mozilla-central/rev/d99de806665d227ad71262933800ef6b069b230a/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js#63-64,we need to check that the lineHeight style is 1.5, and not 1.5px.

Then, you can run the console mocha test by doing `cd devtools/client/webconsole && yarn install && yarn test`


You might need to do `cd devtools/client/netmonitor && yarn install` first, if you have an error running the previous command.

I hope I was clear enough. If I'm not, do not hesitate to ask any question :)

Comment 17

[ewhite7]

how do i check the light Height Style?   expect(secondElementStyle.lineHeight).toBe(1.5)

Comment 18

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #17)
> how do i check the light Height Style?  
> expect(secondElementStyle.lineHeight).toBe(1.5)

yes. that should do

Comment 19

[ewhite7]

ok this is the Errors
 147 passing (2s)
  1 pending
  1 failing

  1) ConsoleAPICall component: console.log renders string grips with custom style:
     Error: Expected undefined to be 1.5
      at assert (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\assert.js:29:9)
      at Expectation.toBe (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\Expectation.js:66:28)
      at Context.<anonymous> (C:/mozilla-source/mozilla-central/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:65:45)
      at callFn (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:326:21)      at Test.Runnable.run (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:422:10)
      at C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:528:12
      at next (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:342:14)
      at C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:352:7
      at next (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (C:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:320:5)



error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.


But it did create a Yarn.lock file

Comment 20

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

(In reply to ewhite7 from comment #19)
> ok this is the Errors
>  147 passing (2s)
>   1 pending
>   1 failing
> 
>   1) ConsoleAPICall component: console.log renders string grips with custom
> style:
>      Error: Expected undefined to be 1.5
>       at assert
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\expect\lib\assert.js:29:9)
>       at Expectation.toBe
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\expect\lib\Expectation.js:66:
> 28)
>       at Context.<anonymous>
> (C:/mozilla-source/mozilla-central/devtools/client/webconsole/new-console-
> output/test/components/console-api-call.test.js:65:45)
>       at callFn
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:326:
> 21)      at Test.Runnable.run
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:319:7)
>       at Runner.runTest
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:422:10)
>       at
> C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:528:12
>       at next
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:342:14)
>       at
> C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:352:7
>       at next
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:284:14)
>       at Immediate.<anonymous>
> (C:\mozilla-source\mozilla-
> central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:320:5)
> 
> 
> 
> error Command failed with exit code 1.
> info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this
> command.
> 
> 
> But it did create a Yarn.lock file

Did you updated the stub ? 
Also, could you push your patch to MozReview (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html)so I can see what's going on ? Thanks !

Comment 21

[ewhite7]

Attachment: myLineChanges.diff

first Patch Attempt

Comment 22

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Oopsie, the url for the mozreview install doc was messed up. Here is the good one : http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html

Comment 23

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 8939717 [details] [diff] [review]
myLineChanges.diff

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

Thanks for posting your WIP.
So now I can see why your test is failing : the line-height property was added as the 4th argument of console.log.
But it should be in the 3rd argument string (see what I suggested in the comment), and then you should run the command to update the stubs (see Comment 16).

Also, could you try to push to mozreview next time ? It makes things easier for both the reviewer and the reviewee :) If you struggle with that, attach another patch here, that's fine for this kind of small patches.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +731,5 @@
>    display: table-row;
>    height: 24px;
>  }
>  
> +.request-list-item:-moz-focusring {

this shouldn't be in this patch

::: devtools/client/netmonitor/yarn.lock
@@ +5682,5 @@
>      watchpack "^1.3.1"
>      webpack-sources "^1.0.1"
>      yargs "^6.0.0"
>  
> +webpack@^3.3.0:

you can revert changes in this file

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js
@@ +161,3 @@
>        }, object);
>      }, {});
> +

can you remove this empty line please ?

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js
@@ +107,5 @@
>  console.log(
>    "%cfoo%cbar",
>    "color:blue;font-size:1.3em;background:url('http://example.com/test');position:absolute;top:10px",
> +  "color:red;background:\\165rl('http://example.com/test')"),
> +  "line-height: 1.5";

let's put this on the previous line, like : 
"color:red;line-height: 1.5;background:\\165rl('http://example.com/test')");

Maybe we can even clean this up a bit with multiline strings : 

console.log(
  "%cfoo%cbar", 
  `color:blue;
   font-size:1.3em;
   background:url('http://example.com/test');
   position:absolute;
   top:10px`,
  `color:red;
   line-height: 1.5;
   background:\\165rl('http://example.com/test')`
);

Comment 24

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 8939717 [details] [diff] [review]
myLineChanges.diff

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

Thanks for posting your WIP.
So now I can see why your test is failing : the line-height property was added as the 4th argument of console.log.
But it should be in the 3rd argument string (see what I suggested in the comment), and then you should run the command to update the stubs (see Comment 16).

Also, could you try to push to mozreview next time ? It makes things easier for both the reviewer and the reviewee :) If you struggle with that, attach another patch here, that's fine for this kind of small patches.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +731,5 @@
>    display: table-row;
>    height: 24px;
>  }
>  
> +.request-list-item:-moz-focusring {

this shouldn't be in this patch

::: devtools/client/netmonitor/yarn.lock
@@ +5682,5 @@
>      watchpack "^1.3.1"
>      webpack-sources "^1.0.1"
>      yargs "^6.0.0"
>  
> +webpack@^3.3.0:

you can revert changes in this file

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js
@@ +161,3 @@
>        }, object);
>      }, {});
> +

can you remove this empty line please ?

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js
@@ +107,5 @@
>  console.log(
>    "%cfoo%cbar",
>    "color:blue;font-size:1.3em;background:url('http://example.com/test');position:absolute;top:10px",
> +  "color:red;background:\\165rl('http://example.com/test')"),
> +  "line-height: 1.5";

let's put this on the previous line, like : 
"color:red;line-height: 1.5;background:\\165rl('http://example.com/test')");

Maybe we can even clean this up a bit with multiline strings : 

console.log(
  "%cfoo%cbar", 
  `color:blue;
   font-size:1.3em;
   background:url('http://example.com/test');
   position:absolute;
   top:10px`,
  `color:red;
   line-height: 1.5;
   background:\\165rl('http://example.com/test')`
);

Comment 25

[ewhite7]

I'm free all this week. can we schedule a meeting on slack? or any other platform and use something like team-viewer

Comment 26

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Sure, what timezone are you in ? Also, you can just ping me on slack and we'll agree on a slot

Comment 27

[ewhite7]

Attachment: Bug 1393609 - Fix line-height custom style for console.log;

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

Comment 28

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 8940248 [details]
Bug 1393609 - Fix line-height custom style for console.log;

https://reviewboard.mozilla.org/r/210546/#review216202

We need to update the stubs to take the change from stub-snippets into account. 
This can be done by running `./mach test devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_console_api.js`

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:149
(Diff revision 1)
> -  // Return a style object as expected by React DOM components, e.g.
> -  // {color: "red"}
> -  // without forbidden properties and values.
> +  const cssToIdlDictionnary = {
> +  "line-height": "lineHeight"
> +  };

we still want to have this comment

Comment 29

[ewhite7]

Comment on attachment 8940248 [details]
Bug 1393609 - Fix line-height custom style for console.log;

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

Comment 30

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 8940248 [details]
Bug 1393609 - Fix line-height custom style for console.log;

https://reviewboard.mozilla.org/r/210546/#review216428

Hey, thanks for going through the whole repo setup again. It seems that the file you saved did change on mozilla-central, so you should only apply your change to those files.
Also, it looks like the command to update the stubs was not run (./mach test devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_console_api.js)

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:1
(Diff revision 2)
> +Stud-snippets

oops

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:150
(Diff revision 2)
> -  // Return a style object as expected by React DOM components, e.g.
> -  // {color: "red"}
> -  // without forbidden properties and values.
> +  const cssToIdlDictionnary = {
> +  "line-height": "lineHeight"
> +  };
> +
>    return [...dummy.style]
>      .filter(name => {
>        return allowedStylesRegex.test(name)
>          && !forbiddenValuesRegexs.some(regex => regex.test(dummy.style[name]));
>      })
>      .reduce((object, name) => {
>        return Object.assign({
> -        [name]: dummy.style[name]
> +        [cssToIdlDictionnary[name] || name]: dummy.style[name]

you should only have these changes in this file.
The other changes are because you pasted an old version of the file, but it has changed on mozilla-central.

To revert the unwanted changes, you can do `hg log` to get the sha1 of the commit.
Then you can use `hg histedit xxx` where xxx is the sha1

For the commit, chose "edit".
Then, use hg record, which will let you only pick what you want.

And then use hg histedit --continue, and I think you should be good.

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js:164
(Diff revision 2)
> -  "inspect({a: 1})",
> +  "inspect({a: 1})"
> -  "cd(document)"

this change shouldn't be made

Comment 31

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Hey Jason, did you resolved your repo issues ?

Comment 32

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

The issue seems to be fixed, I think by the React 16 update.
Let's use this bug to add a test to make sure we don't regress

Comment 33

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Attachment: Bug 1393609 - Add a test for line-height in console.log custom style; .

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

Comment 34

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 8951674 [details]
Bug 1393609 - Add a test for line-height in console.log custom style; .

https://reviewboard.mozilla.org/r/220972/#review226904

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/168429295525
Add a test for line-height in console.log custom style; r=nchevobbe.

Comment 36

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/168429295525

Comment 37

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

clearing needinfo since the bug landed