1307908 - Implement custom styles

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Lin Clark [:linclark]]

Originally posted by:linclark

see https://github.com/devtools-html/gecko-dev/issues/242

In the current webconsole, you can add your own styling to console log messages.

```
console.log('%ccustom styles', 'color:green')
```

We need to reimplement this.

Tests:

- [ ] browser_webconsole_console_custom_styles.js

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1307908 - Implement custom styles in new console frontend. ;

Add the possibility to passe a style object to StringRep.
Parse the styles strings passed as argument to strip off not allowed
properties.
Add a mocha test, and a fixture for parseDeclaration, to ensure custom
styles are rendered as expected.
Since we're using `parseDeclarations`, we need to populate some data by calling
`initCssProperties`, which is async. So we make sure that the `initCssProperties` is
done before calling the init methode on the new console frontend.

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

Comment 2

[Lin Clark [:linclark]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review85304

::: devtools/client/shared/components/reps/string.js:42
(Diff revision 1)
>        let member = this.props.member;
> +      let style = this.props.style;
> +
> +      let config = {className: "objectBox objectBox-string"};
> +      if (style) {
> +        config = Object.assign({}, {style}, config);

Do we need to do Object.assign here? Since the object is created here and not passed in, it seems like it could just set config.style.

::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:66
(Diff revision 1)
>          mode: props.mode,
>        })
>    );
>  }
>  
> +function cleanupStyle(style, parseDeclarations) {

This seems like something that could be generalized. Was there any handling for this before? Is there any similar logic in the rest of the codebase that is/could be generic?

::: devtools/client/webconsole/new-console-output/types.js:39
(Diff revision 1)
>    repeat: 1,
>    repeatId: null,
>    stacktrace: null,
>    frame: null,
>    groupId: null,
> +  styles: [],

Since an array is mutable, I don't know that we should initialize anything to an array. I'd say use null here.

Comment 3

[Lin Clark [:linclark]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

I'm going to ask Patrick and Greg for their thoughts on the CSS stuff since they have worked on similar stuff.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

> This seems like something that could be generalized. Was there any handling for this before? Is there any similar logic in the rest of the codebase that is/could be generic?

There is a similar cleanup function in the current console (http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#1525). Which surprised me because I would have thought that we would be doing the CSS parsing (and cleanup) on the server-side, since it already parse the `%s` string in the log message.

I searched for a similar function in shared utils and the inspector but couldn't find anything, but Patrick and Greg know better for sure.

Comment 5

[Greg Tatum [:gregtatum]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review85304

> This seems like something that could be generalized. Was there any handling for this before? Is there any similar logic in the rest of the codebase that is/could be generic?

I'm not aware of anyone else in devtools doing this specific type of work of validating user input. This function looks pretty good to me. The only thing I would change is maybe add a bit more comments for people not familiar with what's going on with parseDeclaration. Also be aware that this could be performance sensitive as the parser can be a bit slow. I profiled some simple declarations and they were around 3ms on my machine. A loop of 100 runs took 83ms. Something to be aware of if you're being performance sensitive, although maybe that won't matter in this case.

Here is a quick script for testing it in your browser console:

```js
var {require} = Components.utils.import("resource://devtools/shared/Loader.jsm", {});
var {initCssProperties, getClientCssProperties} = require("devtools/shared/fronts/css-properties");
var {parseDeclarations} = require("devtools/shared/css/parsing-utils");
var cssProperties = getClientCssProperties();

console.time("parseDeclarations");
parseDeclarations(cssProperties, "/* margin: 0px; */ padding: 1em; color: #fff; font-size: 1em;");
console.timeEnd("parseDeclarations");
```

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

Tom is a better person than me to give feedback on this specific issue.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review85920

::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:68
(Diff revision 1)
>    );
>  }
>  
> +function cleanupStyle(style, parseDeclarations) {
> +  // Regular expression that matches the allowed CSS property names.
> +  const allowedStylesRegex = /^(?:-moz-)?(?:background|border|box|clear|color|cursor|display|float|font|line|margin|padding|text|transition|outline|white-space|word|writing|(?:min-|max-)?width|(?:min-|max-)?height)/;

I think this part is ok, though I wonder if the regexp should be tighter, for example ending with $

::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:72
(Diff revision 1)
> +  // Regular expression that matches the allowed CSS property names.
> +  const allowedStylesRegex = /^(?:-moz-)?(?:background|border|box|clear|color|cursor|display|float|font|line|margin|padding|text|transition|outline|white-space|word|writing|(?:min-|max-)?width|(?:min-|max-)?height)/;
> +
> +  // Regular expression that matches the forbidden CSS property values.
> +  const forbiddenValuesRegexs = [
> +    // url(), -moz-element()

This isn't going to work as expected, because parseDeclarations doesn't canonicalize escapes in the values -- only in the property names.

For example, I think you can sneak a URL past this using \75rl(...) in the CSS.

parseDeclaration works this way intentionally, because its primary goal is to support as-authored style editing.

One option might be to add a new mode to parseDeclarations.

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:61
(Diff revision 1)
>            return this.toolbox.selectTool("netmonitor").then(panel => {
>              return panel.panelWin.NetMonitorController.inspectRequest(requestId);
>            });
>          },
>          sourceMapService: this.toolbox ? this.toolbox._sourceMapService : null,
> +        parseDeclarations: parseDeclarations.bind(null, getCssProperties(this.toolbox))

Shouldn't this bind to getCssProperties(...).isKnown?

::: devtools/client/webconsole/webconsole.js:496
(Diff revision 1)
> +    if (this.NEW_CONSOLE_OUTPUT_ENABLED) {
> +      let toolbox = gDevTools.getToolbox(this.owner.target)
> +      promises.push(initCssProperties(toolbox));
> +    }
> +
> +    let allReady = Promise.all(promises);

I think this should be "promise.all", not "Promise.all".

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review85920

> This isn't going to work as expected, because parseDeclarations doesn't canonicalize escapes in the values -- only in the property names.
> 
> For example, I think you can sneak a URL past this using \75rl(...) in the CSS.
> 
> parseDeclaration works this way intentionally, because its primary goal is to support as-authored style editing.
> 
> One option might be to add a new mode to parseDeclarations.

FYI, I got those from the current console ( http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#128 & http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#1525 ).
I slightly differs from it as :
- i'm removing all property whose value contains url ( in the old console we replace it with a dull string)
- in the old console, the styles where passed in a dummy node in order to check that the property was allowed ( and I'm using parseDeclaration instead )

I was hoping that parseDeclaraion would be faster than using a node, but Greg suggest it won't, I'll do some tests.

Thanks for your insights

> Shouldn't this bind to getCssProperties(...).isKnown?

it should yes

> I think this should be "promise.all", not "Promise.all".

aren't we supposed to use DOM Promise ?

Comment 9

[Tom Tromey :tromey]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #8)

> > One option might be to add a new mode to parseDeclarations.
> 
> FYI, I got those from the current console (

Ouch.  Well, that seems buggy then.

> > I think this should be "promise.all", not "Promise.all".
> 
> aren't we supposed to use DOM Promise ?

I only mentioned it because the rest of the file is using "promise" and has:

const promise = require("promise");

I don't really have an opinion on which promise we really ought to be using.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

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

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review86280

I only looked at the parts I was asked about earlier.
These look good to me.  Thank you.

Comment 12

[Lin Clark [:linclark]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

https://reviewboard.mozilla.org/r/86138/#review86574

Super minor nit, but otherwise I think this is good. Thanks!

::: devtools/client/webconsole/new-console-output/components/message-types/console-api-call.js:111
(Diff revision 2)
>      dispatch,
>      indent,
>    });
>  }
>  
> -function formatReps(parameters) {
> +function formatReps(parameters, styles, serviceContainer) {

Just a minor nit that I thought of. Could we call this something like userProvidedStyles? I'll be adding a style prop in console-output.js as part of the React Virtualized stuff, so I'm concerned people might not realize those are different things.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8801391 [details]
Bug 1307908 - Implement custom styles in new console frontend. ;

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

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Rebased, made the changes and pushed to TRY (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0432d78da6c5)
Everything looks fine, I'm pushing it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76bf1cbfe6ab
Implement custom styles in new console frontend. r=linclark,tromey;

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76bf1cbfe6ab