918716 - Add color swatches to devtools output parser

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The color swatches should be small circles to the left of each color.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-color-swatches-to-output-parser-918716.patch

As we discussed at the Summit, this is the simple output parser that I have created. What do you guys think? It is fairly simple and I have made it easy to build upon.

Of course, REGEX_CUBIC_BEZIER is not used yet but I have added parts of it to illustrate how simple it is to do.

In the markup view we use:
if (typeof aAttr.value !== "undefined") {
  let outputParser = new OutputParser();
  let frag = outputParser.parseHTMLAttribute(collapsedValue);
  val.appendChild(frag);
}

Maybe we shouldn't use collapsedValue here ... we could find a new way to shorten stuff.

The computed view is a little longer:
let outputParser = new OutputParser();
let frag = outputParser.parseCssProperty(this.propertyInfo.name,
this.propertyInfo.value, {
  colorSwatchClass: "computedview-colorswatch"
});
this.valueNode.innerHTML = "";
this.valueNode.appendChild(frag);

The rule view is fairly similar:
let outputParser = new OutputParser();
let frag = outputParser.parseCssProperty(name, val, {
  colorSwatchClass: "ruleview-colorswatch",
  defaultColorType: !propDirty
});
this.valueSpan.innerHTML = "";
this.valueSpan.appendChild(frag);

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 814420 [details] [diff] [review]
add-color-swatches-to-output-parser-918716.patch

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

::: browser/devtools/styleinspector/computed-view.js
@@ +1092,5 @@
> +  get outputFragment()
> +  {
> +    let outputParser = new OutputParser();
> +    let text = this.selectorInfo.rule.cssText;
> +    let name = text.substr(0, text.indexOf(":"));

I don't think cssText.substr is what you want here.  If the cssText was: "height: 192px; width: 192px; color: red", name would be 'height' even if selectorInfo.value is `red`.

It would be better to modify getMatchedSelectors in the styles.js actor to return a 'name' object and just use that instead of parsing out the string.

::: toolkit/devtools/output-parser.js
@@ +133,5 @@
> +    this.parsed.length = 0;
> +    let dirty = false;
> +    let nameValueSupported = false;
> +
> +    let matched = function(match) {

This whole block took me a while to figure out and is a little tricky.  Either (a) the 'matched' function name here could be more descriptive, something like removeMatchFromText or something, or (b) is there a way to accomplish the same thing without sharing state between each match?  This could maybe be accomplished without the loop by using the /g/ flag on the regexes? Maybe just run each regex globally, then store the index of the match along with a DOM node, rather than directly appending at the time of the match.  Once all of them have been tested they could be stored inside of this.parsed in the proper order.  This could clear up some of the state being shared between all of the functions.  To be more clear, this._appendColor would be called this._getColorNode or something and it would just return a node.  So you would do something like this (there is probably a better way to do this, just off the top of my head):

this.matches = {}
text.replace(REGEX_QUOTES, (match, offset) => {
  this.matches[offset] = this._getTextNode(match);
  return match;
});

// .... and so on

// Sort matches by offset then one by one append to this.parsed in order.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 814420 [details] [diff] [review]
> add-color-swatches-to-output-parser-918716.patch
> 
> Review of attachment 814420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/computed-view.js
> @@ +1092,5 @@
> > +  get outputFragment()
> > +  {
> > +    let outputParser = new OutputParser();
> > +    let text = this.selectorInfo.rule.cssText;
> > +    let name = text.substr(0, text.indexOf(":"));
> 
> I don't think cssText.substr is what you want here.  If the cssText was:
> "height: 192px; width: 192px; color: red", name would be 'height' even if
> selectorInfo.value is `red`.
> 
> It would be better to modify getMatchedSelectors in the styles.js actor to
> return a 'name' object and just use that instead of parsing out the string.
> 

Yup, agreed.

> ::: toolkit/devtools/output-parser.js
> @@ +133,5 @@
> > +    this.parsed.length = 0;
> > +    let dirty = false;
> > +    let nameValueSupported = false;
> > +
> > +    let matched = function(match) {
> 
> This whole block took me a while to figure out and is a little tricky. 
> Either (a) the 'matched' function name here could be more descriptive,
> something like removeMatchFromText or something,

That would make sense.

> or (b) is there a way to
> accomplish the same thing without sharing state between each match?  This
> could maybe be accomplished without the loop by using the /g/ flag on the
> regexes? Maybe just run each regex globally, then store the index of the
> match along with a DOM node, rather than directly appending at the time of
> the match.  Once all of them have been tested they could be stored inside of
> this.parsed in the proper order.  This could clear up some of the state
> being shared between all of the functions.  To be more clear,
> this._appendColor would be called this._getColorNode or something and it
> would just return a node.  So you would do something like this (there is
> probably a better way to do this, just off the top of my head):
> 
> this.matches = {}
> text.replace(REGEX_QUOTES, (match, offset) => {
>   this.matches[offset] = this._getTextNode(match);
>   return match;
> });
> 
> // .... and so on
> 
> // Sort matches by offset then one by one append to this.parsed in order.

This is a pretty standard pattern for a parser:
1. if text length > 0
2.   Search for a match from the start of the text and bite it off.
3.   Process it by manipulating it e.g. creating dom nodes.
4.   Queue the results for output
5.   If there were no matches go to 1
6.   else gather output and return it.

Using global regexes in a parser is very dangerous because you would turn out parsing the same text multiple times and this becomes very dangerous as a parser grows. Besides, sharing state is very important in a parser e.g. we use it here to identify that a property name has been parsed and prepare to receive a property value (e.g. only showing colors for color related properties).

As it is, adding something to the parser simply requires adding a new rule and a method to create any elements.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed feedback

I have addressed Brian's comments and simplified the code.

I think it was confusing because I used text.replace to match regexes and run functions based on the results. We now check for a regex match and act based on the result, making our intentions much clearer.

Just need to fix tests now.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed various issues

Seems rock solid now and passes all tests.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=8f9a2b768745

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 816758 [details] [diff] [review]
Addressed various issues

That should have been "rock solid on OSX" ... I will fix the issue on other OSes before requesting review.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Use span instead of div to fix platform specific copy paste

Fixed.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d8a9e7bd6a1d

API Etherpad:
https://etherpad.mozilla.org/devtools-output-parser-api

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 817113 [details] [diff] [review]
Use span instead of div to fix platform specific copy paste

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

::: toolkit/devtools/output-parser.js
@@ +22,5 @@
> + *  - hsl()
> + *  - hsla()
> + *  - rgb()
> + *  - rgba()
> + *  - red

red?

@@ +35,5 @@
> +loader.lazyGetter(this, "REGEX_ALL_CSS_PROPERTIES", function () {
> +  let names = DOMUtils.getCSSPropertyNames();
> +    let pattern = "^(";
> +
> +    for (let i = 0, len = names.length; i < len; i++) {

Is this performance optimization worth the decrease in readability?

@@ +88,5 @@
> +   */
> +  parseCssProperty: function(name, value, options={}) {
> +    options = this._mergeOptions(options);
> +    options.cssPropertyName = name;
> +    options.colors = this._cssPropertySupportsValue(name, "red");

Needs a comment, maybe:

// Detect if 'name' supports colors by checking if papayawhip is a valid value

@@ +89,5 @@
> +  parseCssProperty: function(name, value, options={}) {
> +    options = this._mergeOptions(options);
> +    options.cssPropertyName = name;
> +    options.colors = this._cssPropertySupportsValue(name, "red");
> +    options.cubicBezier = this._cssPropertySupportsValue(name, "cubic-bezier");

Don't we want to check for timing functions in general?

@@ +229,5 @@
> +  _cssPropertySupportsValue: function(propertyName, propertyValue) {
> +    let autoCompleteValues = DOMUtils.getCSSValuesForProperty(propertyName);
> +
> +    // For colors check for valid color.
> +    if (autoCompleteValues.indexOf("rgb") !== -1) {

I wonder if using something less common might lead to fewer false positives. For example

    autoCompleteValues.indexOf("papayawhip")

Whatever, this code looks bizarre, so could we make the comment clearer:

// Detect if propertyName supports colors by checking if papayawhip is a valid value

@@ +278,5 @@
> +   *         _mergeOptions().
> +   */
> +  _appendURL: function(match, url, line, options={}) {
> +    this._appendTextNode(match);
> +    // FIXME: ADD URL ONCLICK HANDLERS

Could we remove all the commented out code please.

@@ +407,5 @@
> +    return new colorUtils.CssColor(color).valid;
> +  },
> +
> +  /**
> +   * Merges options objects. Default values are set here.

Could you document what the options are and what they do?

@@ +431,5 @@
> +};
> +
> +loader.lazyGetter(this, "DOMUtils", function () {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});

Normally we put the imports at the top, does this need to be at the bottom?

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now with bounteous use of papayawhip

(In reply to Joe Walker [:jwalker] from comment #8)
> Comment on attachment 817113 [details] [diff] [review]
> Use span instead of div to fix platform specific copy paste
> 
> Review of attachment 817113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/output-parser.js
> @@ +22,5 @@
> > + *  - hsl()
> > + *  - hsla()
> > + *  - rgb()
> > + *  - rgba()
> > + *  - red
> 
> red?
> 

I have changed this to say color name. If it is a valid color we process it as a color, otherwise we process it as text.

> @@ +35,5 @@
> > +loader.lazyGetter(this, "REGEX_ALL_CSS_PROPERTIES", function () {
> > +  let names = DOMUtils.getCSSPropertyNames();
> > +    let pattern = "^(";
> > +
> > +    for (let i = 0, len = names.length; i < len; i++) {
> 
> Is this performance optimization worth the decrease in readability?
> 

Changed

> @@ +88,5 @@
> > +   */
> > +  parseCssProperty: function(name, value, options={}) {
> > +    options = this._mergeOptions(options);
> > +    options.cssPropertyName = name;
> > +    options.colors = this._cssPropertySupportsValue(name, "red");
> 
> Needs a comment, maybe:
> 
> // Detect if 'name' supports colors by checking if papayawhip is a valid
> value
> 

Done

> @@ +89,5 @@
> > +  parseCssProperty: function(name, value, options={}) {
> > +    options = this._mergeOptions(options);
> > +    options.cssPropertyName = name;
> > +    options.colors = this._cssPropertySupportsValue(name, "red");
> > +    options.cubicBezier = this._cssPropertySupportsValue(name, "cubic-bezier");
> 
> Don't we want to check for timing functions in general?
> 

Yes, we do, but not at this point. This was left in so that Patrick could see what needs to be done to support cubic bezier (we plan a doorhanger and maybe even a widget for that). I have commented out the line.

> @@ +229,5 @@
> > +  _cssPropertySupportsValue: function(propertyName, propertyValue) {
> > +    let autoCompleteValues = DOMUtils.getCSSValuesForProperty(propertyName);
> > +
> > +    // For colors check for valid color.
> > +    if (autoCompleteValues.indexOf("rgb") !== -1) {
> 
> I wonder if using something less common might lead to fewer false positives.

There are no false positives. If rgb is provided as an autocomplete value the property supports colors.

> For example
> 
>     autoCompleteValues.indexOf("papayawhip")
> 
> Whatever, this code looks bizarre, so could we make the comment clearer:
> 
> // Detect if propertyName supports colors by checking if papayawhip is a
> valid value
> 

Any excuse to use papayawhip and I will take it... at least until paulrougetpink is standardized.

Done.

> @@ +278,5 @@
> > +   *         _mergeOptions().
> > +   */
> > +  _appendURL: function(match, url, line, options={}) {
> > +    this._appendTextNode(match);
> > +    // FIXME: ADD URL ONCLICK HANDLERS
> 
> Could we remove all the commented out code please.
> 

It was there so that Patrick doesn't have to write it again... removed.

> @@ +407,5 @@
> > +    return new colorUtils.CssColor(color).valid;
> > +  },
> > +
> > +  /**
> > +   * Merges options objects. Default values are set here.
> 
> Could you document what the options are and what they do?
> 

Done

> @@ +431,5 @@
> > +};
> > +
> > +loader.lazyGetter(this, "DOMUtils", function () {
> > +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> > +});
> 
> Normally we put the imports at the top, does this need to be at the bottom?

Not everywhere, but if we have a standard I am happy to stick with it... moved.

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 817852 [details] [diff] [review]
Now with bounteous use of papayawhip

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

Thanks. I'm going to assume that Patrick is happy with this API since he will be the primary other user of it?

::: browser/devtools/markupview/markup-view.js
@@ +1555,2 @@
>  
> +  _truncateFrag: function(frag) {

Please could you document why we need to do this truncation?

::: toolkit/devtools/output-parser.js
@@ +74,5 @@
> +
> +exports.OutputParser = OutputParser;
> +
> +OutputParser.prototype = {
> +  parsed: [],

Array shared on prototype = running with scissors?

@@ +134,5 @@
> +   *         A document fragment containing events etc. Colors will not be
> +   *         parsed.
> +   */
> +  _parse: function(text, options={}) {
> +    options = this._mergeOptions(options);

I don't think we need this since _parse is internal?
(same comment applies to other internal functions)

@@ +197,5 @@
> +          trimMatchFromStart(match);
> +          this._appendColor(match, options);
> +        }
> +
> +        /*matched = text.match(REGEX_CUBIC_BEZIER);

It could easily be a while before we support doorhangers for cubic beziers. I'm guessing that 99% of people never change from the default, and 99% of those that do will find no need for a custom function. So I suggest creating a bug for adding cubic bezier support and attaching a patch to that, and then paring this down completely.

@@ +321,5 @@
> +   * @param  {String} [value]
> +   *         If a value is included it will be appended as a text node inside
> +   *         the tag. This is useful e.g. for span tags.
> +   * @return {Node}
> +   *         DOM Node (to allow chaining)

Many of the _append functions don't return a Node and so can't be chained, so perhaps we should be consistent? Since we don't actually ever use chaining, maybe the best thing to do is to not return anything anywhere?

@@ +392,5 @@
> +   *
> +   * @param  {String} color
> +   *         Color to check
> +   */
> +  _colorIsValid: function(color) {

Since this is a question, perhaps isColorValid would be a better name?

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed reviewers comments

(In reply to Joe Walker [:jwalker] from comment #10)
> Comment on attachment 817852 [details] [diff] [review]
> Now with bounteous use of papayawhip
> 
> Review of attachment 817852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. I'm going to assume that Patrick is happy with this API since he
> will be the primary other user of it?
> 
> ::: browser/devtools/markupview/markup-view.js
> @@ +1555,2 @@
> >  
> > +  _truncateFrag: function(frag) {
> 
> Please could you document why we need to do this truncation?
> 

Done

> ::: toolkit/devtools/output-parser.js
> @@ +74,5 @@
> > +
> > +exports.OutputParser = OutputParser;
> > +
> > +OutputParser.prototype = {
> > +  parsed: [],
> 
> Array shared on prototype = running with scissors?
> 

OMG: Did I really do that? Madness, that is what I call it!

Fixed

> @@ +134,5 @@
> > +   *         A document fragment containing events etc. Colors will not be
> > +   *         parsed.
> > +   */
> > +  _parse: function(text, options={}) {
> > +    options = this._mergeOptions(options);
> 
> I don't think we need this since _parse is internal?
> (same comment applies to other internal functions)
> 

Well spotted, removed from all internal functions.

> @@ +197,5 @@
> > +          trimMatchFromStart(match);
> > +          this._appendColor(match, options);
> > +        }
> > +
> > +        /*matched = text.match(REGEX_CUBIC_BEZIER);
> 
> It could easily be a while before we support doorhangers for cubic beziers.
> I'm guessing that 99% of people never change from the default, and 99% of
> those that do will find no need for a custom function. So I suggest creating
> a bug for adding cubic bezier support and attaching a patch to that, and
> then paring this down completely.
> 

I have just removed it as it is a simple job to add it again.

> @@ +321,5 @@
> > +   * @param  {String} [value]
> > +   *         If a value is included it will be appended as a text node inside
> > +   *         the tag. This is useful e.g. for span tags.
> > +   * @return {Node}
> > +   *         DOM Node (to allow chaining)
> 
> Many of the _append functions don't return a Node and so can't be chained,
> so perhaps we should be consistent? Since we don't actually ever use
> chaining, maybe the best thing to do is to not return anything anywhere?
> 

Agreed, append methods now return nothing.

> @@ +392,5 @@
> > +   *
> > +   * @param  {String} color
> > +   *         Color to check
> > +   */
> > +  _colorIsValid: function(color) {
> 
> Since this is a question, perhaps isColorValid would be a better name?

Changed

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2601d36dc1f1

Comment 13

[Matt Brubeck (:mbrubeck)]

Sorry, backed out because of test failures in browser-inspector-changes.js:
https://hg.mozilla.org/integration/fx-team/rev/e5b6c39b7fbe

https://tbpl.mozilla.org/php/getParsedLog.php?id=29269756&tree=Fx-Team

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-color-swatches-to-output-parser-918716.patch

Fixed failing test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=8d35aa1c1535

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/54355463e47d

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/54355463e47d