Closed Bug 918716 Opened 11 years ago Closed 11 years ago

Add color swatches to devtools output parser

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: miker, Assigned: miker)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

The color swatches should be small circles to the left of each color.
Blocks: 918738
No longer depends on: 918738
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);
Attachment #814420 - Flags: feedback?(pbrosset)
Attachment #814420 - Flags: feedback?(bgrinstead)
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.
Attachment #814420 - Flags: feedback?(bgrinstead) → feedback+
Blocks: 706102
Blocks: 889638
(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.
Attached patch Addressed feedback (obsolete) — Splinter Review
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.
Attachment #814420 - Attachment is obsolete: true
Attachment #814420 - Flags: feedback?(pbrosset)
Attached patch Addressed various issues (obsolete) — Splinter Review
Seems rock solid now and passes all tests.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=8f9a2b768745
Attachment #815913 - Attachment is obsolete: true
Attachment #816758 - Flags: review?(jwalker)
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.
Attachment #816758 - Flags: review?(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?
Attachment #817113 - Flags: review?(jwalker)
(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.
Attachment #817113 - Attachment is obsolete: true
Attachment #817852 - Flags: review?(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?
Attachment #817852 - Flags: review?(jwalker) → review+
Attached patch Addressed reviewers comments (obsolete) — Splinter Review
(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
Attachment #817852 - Attachment is obsolete: true
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/2601d36dc1f1
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Blocks: 765105
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/54355463e47d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Depends on: 928767
Depends on: 938196
Depends on: 944731
Depends on: 1003424
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.