929384 - Color swatch missing for background properties with href

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

This is a follow-up bug for bug 918716.
The new color swatches in the DevTools' rule and computed views look great!

I think there's one case missing tough: 

> background: red url("http://something.com/image.png") no-repeat top left;

In this case, the "red" color part will not get a color swatch next to it.
This will work though:

> background: red url("") no-repeat top left;

It seems that the color parsing in rule-view.js specifically ignores declarations that have URIs.

Comment 1

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

Attachment: color-swatch-missing-with-href-929384.patch

When I first saw this I thought it would be a simple fix but it seems like I was wrong. This is one of those bugs where a whole bunch of issues need to be fixed in order to get things up and running properly.

I have added a test for 'background: url(\"chrome://branding/content/about-logo.png\") repeat scroll 0% 0% red' as it was the original problem reported in this bug. To fix this I have also addressed the following issues:

- outputParser now has a destructor that removes any events that it has added to nodes.
- outputParser now uses the HTML namespace in order to append nodes. This fixes scroll issues when adding nodes to HTML documents.
- outputParser parser method has been simplified and the logic to check whether a css property supports a particular value has been moved into parseCssProperty().
- _cssPropertySupportsValue() no longer uses css autocompletion values to check for validity as that method is too error prone.
- outputParser now parses url(xxx) including relative URLs into links. This means that two new options needed to be added: urlClass and baseURI.
- The colors and cssPropertyName options have been removed from outputParser as they are not needed. Color swatches are displayed only if colorSwatchClass is set, colors are converted to default only if defaultColorType is set.

- _truncateFrag now works correctly in all cases. It previously overcropped in some situations.
- Changed properties now remain changed in the rule view, even when moving away from and back to an element.
- The rule view no longer uses promises when applying modifications. This is because a number of race conditions and does not cause any issues.
- The rule view no longer needs to process properties containing URLs separately.
- In some circumstances we became confused about which value was actually displayed for a property in the rule view. We now simply use the value span's textContent instead.
- The rule view store sometimes returned the wrong property value. This is now fixed.

Comment 2

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

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 824870 [details] [diff] [review]
color-swatch-missing-with-href-929384.patch

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

I'm basically ok with this patch, a few comments you'll see below and 2 more important things:

- I haven't seen a test case that tests fragments are being returned correctly, with color swatches at the right places.
- I'm a bit worried about the parsing process being coupled with the, increasingly complex, DOM generation/event listeners attachment phase. For now, it's only about clicking links, but with the new tooltips I'm working on, this will get more complex.

This last point, however, is a more of longer term discussion we need to have I guess, and one that's linked to our discussion with Cameron about parsing authored styles, so we can perhaps tackle this after this bug.
Let me know and I'll R+

::: browser/devtools/styleinspector/rule-view.js
@@ +562,5 @@
>      } else {
>        disabled.delete(this.style);
>      }
>  
> +    let promise = aModifications.apply();

This seems to work and actually fixes a bug I've been having with the new color picker tooltip I'm working on, but I don't fully understand what's going on here. You mentioned race conditions.
I think a comment would be good here, indeed this is a crucial function that is both getting pretty long and hard to understand.

@@ +2183,5 @@
>        // A new property should be removed when escape is pressed.
>        if (this.removeOnRevert) {
>          this.remove();
>        } else {
> +        this.prop.setValue(this.valueSpan.textContent, this.committed.priority);

Can you explain in which circumstances this was required. I have done some simple tests with both your change and the initial code and saw no differences.

this.committed.value seems to hold the initial value of the property, so I don't see why we need to change this, although for sure your fix would work.

::: browser/devtools/styleinspector/test/browser_ruleview_update.js
@@ +156,5 @@
>  
> +  inspector.once("rule-view-refreshed", testPropertyChange7);
> +
> +  // Add an entirely new property
> +  testElement.setAttribute("style", "background: url(\"chrome://branding/content/about-logo.png\") repeat scroll 0% 0% red");

I see there's a new test case for the problem I originally reported in the bug, but I don't see how you test that a color swatch has been generated. Did I miss another test case specifically targeted at asserting fragments are created correctly (basically testing the parser's output)?

::: toolkit/devtools/output-parser.js
@@ +169,3 @@
>        matched = text.match(REGEX_ALL_CSS_PROPERTIES);
>        if (matched) {
> +        let [match,] = matched;

Nit: maybe you should get rid of the trailing comma

@@ +217,5 @@
>  
> +    let div = doc.createElement("div");
> +    div.style[name] = value;
> +
> +    return !!div.style[name];

This implementation of _cssPropertySupportsValue seems indeed a lot better.

@@ +248,2 @@
>      }
> +    return false;

_appendColor is the only one of the _append* functions that returns a boolean.
I would either document this return value in the jsdoc, or (preferably) make it consistent with the other _append* functions by removing the return value, and first calling the _isColorValid function that does the same job (which by the way seems unused right now).

@@ +276,5 @@
> +        class: options.urlClass,
> +        href: href
> +      }, url);
> +
> +      this._addEventListener(a, "click", (event) => {

Here you're basically delegating the interaction with fragments to the parser.
Although it seems fine for links here, I have 2 sort of problems with this:

1) the parser should really be concerned about parsing, adding event listeners, or creating doorhangers, etc. should come in a second step, after the parsing is done. This would simplify the output-parser a bit and make it easier to use its generated fragments in a number of different ways. Indeed, we might not always want the image URL to open in a new window, depending on where the fragment is used in the devtools.

2) it will make the output-parser gradually have to know more and more of the rest of the devtools. For instance, I'm working on the color-picker doorhanger right now. And to make this works, when I instantiate the color-picker, I need to wire a few event listeners to live preview a color change, commit it or revert it. That means what I'm doing is, in the rule-view, getting the fragment, finding the swatches in that fragment, adding click event listeners on them to trigger the color-picker. And since I'm doing this in the rule-view, I can then just add event listeners to the color-picker color change to call the private _livePreview function for instance.
I wouldn't really be able to do this easily if the whole code were to be in output-parser.

I'd like you're view on this though.

If you think that creation of doorhangers like the color-picker should be done by the output-parser, at least we need to add options like {onColorPickerColorChange: cb, onColorPickerColorRevert: cb, onColorPickerColorCommit: cb}, etc...

I'm ok either way I guess, but I would really see the parsing and decorating phases being separated. 1) the parsing phase would not return fragments, but an object model so that 2) we can then run the decoration phase to read this model and create swatches, add event listeners, tooltips, etc. and return fragments.

This in fact would even work better with our longer term plan to use gecko's parser to parse authored styles. Indeed, when we'll be able to do this, the parsing/model generation phase would be handled by gecko, right?

Comment 4

[Joe Walker [:jwalker]]

> @@ +248,2 @@
> >      }
> > +    return false;
> 
> _appendColor is the only one of the _append* functions that returns a
> boolean.
> I would either document this return value in the jsdoc, or (preferably) make
> it consistent with the other _append* functions by removing the return
> value, and first calling the _isColorValid function that does the same job
> (which by the way seems unused right now).

I've noted this before too, but I can't remember where. Mike, was it in this file or somewhere else - I wonder how many places we have this problem?

Comment 5

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

(In reply to Joe Walker [:jwalker] from comment #4)
> > @@ +248,2 @@
> > >      }
> > > +    return false;
> > 
> > _appendColor is the only one of the _append* functions that returns a
> > boolean.
> > I would either document this return value in the jsdoc, or (preferably) make
> > it consistent with the other _append* functions by removing the return
> > value, and first calling the _isColorValid function that does the same job
> > (which by the way seems unused right now).
> 
> I've noted this before too, but I can't remember where. Mike, was it in this
> file or somewhere else - I wonder how many places we have this problem?

It was this file but I had to add the return in order to allow adding listeners to a node. Of course, this means that all append* methods should return their output.

Comment 6

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

Attachment: color-swatch-missing-with-href-929384.patch

(In reply to Patrick Brosset from comment #3)
> Comment on attachment 824870 [details] [diff] [review]
> color-swatch-missing-with-href-929384.patch
> 
> Review of attachment 824870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm basically ok with this patch, a few comments you'll see below and 2 more
> important things:
> 
> - I haven't seen a test case that tests fragments are being returned
> correctly, with color swatches at the right places.

I address this below

> - I'm a bit worried about the parsing process being coupled with the,
> increasingly complex, DOM generation/event listeners attachment phase. For
> now, it's only about clicking links, but with the new tooltips I'm working
> on, this will get more complex.
> 
> This last point, however, is a more of longer term discussion we need to
> have I guess, and one that's linked to our discussion with Cameron about
> parsing authored styles, so we can perhaps tackle this after this bug.

The problem is that parsing is necessary in order to display placeholders in the correct location (not just next to the css line but next to the value in the shortcut property). The other issue is that we would turn out with a lot of code duplication of code in each of our tools that use tooltips. If this code was in the output parser it would only be there once and it would be far easier to maintain.

If we are clever about the way we do it then it doesn't need to make the parser much more complex at all. Adding a couple of extra options would be the way to go.

As you say, we can discuss this another time.

> Let me know and I'll R+
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +562,5 @@
> >      } else {
> >        disabled.delete(this.style);
> >      }
> >  
> > +    let promise = aModifications.apply();
> 
> This seems to work and actually fixes a bug I've been having with the new
> color picker tooltip I'm working on, but I don't fully understand what's
> going on here. You mentioned race conditions.
> I think a comment would be good here, indeed this is a crucial function that
> is both getting pretty long and hard to understand.
> 

This introduced new bugs and it seems that the original problem was fixed by another part of this patch so I have reverted this hunk.

> @@ +2183,5 @@
> >        // A new property should be removed when escape is pressed.
> >        if (this.removeOnRevert) {
> >          this.remove();
> >        } else {
> > +        this.prop.setValue(this.valueSpan.textContent, this.committed.priority);
> 
> Can you explain in which circumstances this was required. I have done some
> simple tests with both your change and the initial code and saw no
> differences.
> 
> this.committed.value seems to hold the initial value of the property, so I
> don't see why we need to change this, although for sure your fix would work.
> 

This is needed for one particular situation:
- Inspect an element that has color set
- Open the rule view
- Start changing the color value and press escape.
- The original color will be displayed in rgb format (not hex or whatever is selected).

This is because this value is supplied by the actor and it doesn't know which format is selected on the client.

> ::: browser/devtools/styleinspector/test/browser_ruleview_update.js
> @@ +156,5 @@
> >  
> > +  inspector.once("rule-view-refreshed", testPropertyChange7);
> > +
> > +  // Add an entirely new property
> > +  testElement.setAttribute("style", "background: url(\"chrome://branding/content/about-logo.png\") repeat scroll 0% 0% red");
> 
> I see there's a new test case for the problem I originally reported in the
> bug, but I don't see how you test that a color swatch has been generated.
> Did I miss another test case specifically targeted at asserting fragments
> are created correctly (basically testing the parser's output)?
> 

You are right... the parser is big and ugly enough to have it's own test now.

Created browser/devtools/shared/test/browser_outputparser.js

> ::: toolkit/devtools/output-parser.js
> @@ +169,3 @@
> >        matched = text.match(REGEX_ALL_CSS_PROPERTIES);
> >        if (matched) {
> > +        let [match,] = matched;
> 
> Nit: maybe you should get rid of the trailing comma
> 

Done

> @@ +217,5 @@
> >  
> > +    let div = doc.createElement("div");
> > +    div.style[name] = value;
> > +
> > +    return !!div.style[name];
> 
> This implementation of _cssPropertySupportsValue seems indeed a lot better.
> 

Yes, I was shocked when I realized how error prone the previous implementation was.

> @@ +248,2 @@
> >      }
> > +    return false;
> 
> _appendColor is the only one of the _append* functions that returns a
> boolean.
> I would either document this return value in the jsdoc, or (preferably) make
> it consistent with the other _append* functions by removing the return
> value, and first calling the _isColorValid function that does the same job
> (which by the way seems unused right now).
> 

I had to add the return because we need access to the node after appending it in order that we can add event listeners... _appendURL makes use of this. I have added this to the jsdoc comment and removed _isColorValid.

> @@ +276,5 @@
> > +        class: options.urlClass,
> > +        href: href
> > +      }, url);
> > +
> > +      this._addEventListener(a, "click", (event) => {
> 
> Here you're basically delegating the interaction with fragments to the
> parser.
> Although it seems fine for links here, I have 2 sort of problems with this:
> 
> 1) the parser should really be concerned about parsing, adding event
> listeners, or creating doorhangers, etc. should come in a second step, after
> the parsing is done. This would simplify the output-parser a bit and make it
> easier to use its generated fragments in a number of different ways. Indeed,
> we might not always want the image URL to open in a new window, depending on
> where the fragment is used in the devtools.
> 
> 2) it will make the output-parser gradually have to know more and more of
> the rest of the devtools. For instance, I'm working on the color-picker
> doorhanger right now. And to make this works, when I instantiate the
> color-picker, I need to wire a few event listeners to live preview a color
> change, commit it or revert it. That means what I'm doing is, in the
> rule-view, getting the fragment, finding the swatches in that fragment,
> adding click event listeners on them to trigger the color-picker. And since
> I'm doing this in the rule-view, I can then just add event listeners to the
> color-picker color change to call the private _livePreview function for
> instance.
> I wouldn't really be able to do this easily if the whole code were to be in
> output-parser.
> 
> I'd like you're view on this though.
> 
> If you think that creation of doorhangers like the color-picker should be
> done by the output-parser, at least we need to add options like
> {onColorPickerColorChange: cb, onColorPickerColorRevert: cb,
> onColorPickerColorCommit: cb}, etc...
> 
> I'm ok either way I guess, but I would really see the parsing and decorating
> phases being separated. 1) the parsing phase would not return fragments, but
> an object model so that 2) we can then run the decoration phase to read this
> model and create swatches, add event listeners, tooltips, etc. and return
> fragments.
> 

I agree with you up to a point. The problem is that we should be duplicating as little of our code as possible. I can see this being a two step system so maybe that could be version 2 of the parser.

> This in fact would even work better with our longer term plan to use gecko's
> parser to parse authored styles. Indeed, when we'll be able to do this, the
> parsing/model generation phase would be handled by gecko, right?

Agreed.

Comment 7

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

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

Comment 8

[Patrick Brosset <:pbro>]

I'm yet to review that patch, but already based on your comments here, I think we agree, let's try and get this out there and we'll see later for a v2.
In any case, I'm working on the color picker tooltip right now, and whenever your patch lands, I'll merge it in and we'll see how the output-parser behaves with that use case.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 828088 [details] [diff] [review]
color-swatch-missing-with-href-929384.patch

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

R+ for this patch, granted that the try build is green (and I also made a few comments below).

About the parsing/event listeners discussion we had, I don't really see the point about code duplication. Indeed, I think we can avoid it even if the output-parser is not the one adding event listeners to the fragments' elements.

Let's take the case of the color picker:

- the color picker is going to be a shared widget that requires very little code to instantiate, so if we need it in 5 different tools, those tools won't need to duplicate anything
- still the color picker will require a few initialization properties like: onRevert, onPreview, onCommit, which would be 3 callbacks that the color picker widget would execute whenever a color is chosen or changed or reverted in its UI. So if we want to make the color picker shareable, each tool that needs it will have to supply its own callbacks here.
- so my point is, if the output-parser takes care of both outputting fragments and making them do things, it means in the case of color swatches that it will have to not only add the click event listener but also know about these 3 specific callback parameters. And that's just for the color swatch/picker. There will be more of this as we go forward.

That's why my point was let's try and have a output-parser that generates a fragment, but also give meta-data information about the fragment so that tools can then link the generated swatches (or url links, or whatever may have been generated) with other widgets like color pickers, etc ...

But again, for V2.

::: browser/devtools/shared/test/browser_outputparser.js
@@ +27,5 @@
> +  testParseCssProperty();
> +}
> +
> +function testParseCssProperty() {
> +  let frag = parser.parseCssProperty("color", "red", {

That test is fine, I would just add another one with a more complex property like : "border: 1px solid red" and assert that the color swatch does appear at the right position.

::: browser/devtools/styleinspector/rule-view.js
@@ +2188,5 @@
>        if (this.removeOnRevert) {
>          this.remove();
>        } else {
> +        //this.prop.setValue(this.committed.value, this.committed.priority);
> +        this.prop.setValue(this.valueSpan.textContent, this.committed.priority);

Nit: The commented out line of code should be removed. And by the way, I would probably add a comment explaining why this.committed.value isn't used here as you explained.

::: toolkit/devtools/output-parser.js
@@ +248,2 @@
>      }
> +    return false;

In my last review, when I said there should be a jsdoc comment, I was referring to the _appendColor function. It returns a boolean but the documentation doesn't say so and doesn't explain why it returns it.
I would have personally preferred to keep the isColorValid function because it's an "isSomething" type of function so it's very obvious it returns a boolean, and I would have removed the return statements from _appendColor. But it's not a big deal either, so either way, I'm fine.

Comment 10

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

Attachment: color-swatch-missing-with-href-929384.patch

This needed quite a rebase so I fixed a few more issues:
(In reply to Patrick Brosset from comment #9)
> Comment on attachment 828088 [details] [diff] [review]
> color-swatch-missing-with-href-929384.patch
> 
> Review of attachment 828088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+ for this patch, granted that the try build is green (and I also made a
> few comments below).
> 
> About the parsing/event listeners discussion we had, I don't really see the
> point about code duplication. Indeed, I think we can avoid it even if the
> output-parser is not the one adding event listeners to the fragments'
> elements.
> 
> Let's take the case of the color picker:
> 
> - the color picker is going to be a shared widget that requires very little
> code to instantiate, so if we need it in 5 different tools, those tools
> won't need to duplicate anything
> - still the color picker will require a few initialization properties like:
> onRevert, onPreview, onCommit, which would be 3 callbacks that the color
> picker widget would execute whenever a color is chosen or changed or
> reverted in its UI. So if we want to make the color picker shareable, each
> tool that needs it will have to supply its own callbacks here.
> - so my point is, if the output-parser takes care of both outputting
> fragments and making them do things, it means in the case of color swatches
> that it will have to not only add the click event listener but also know
> about these 3 specific callback parameters. And that's just for the color
> swatch/picker. There will be more of this as we go forward.
> 
> That's why my point was let's try and have a output-parser that generates a
> fragment, but also give meta-data information about the fragment so that
> tools can then link the generated swatches (or url links, or whatever may
> have been generated) with other widgets like color pickers, etc ...
> 

I do agree and understand the problem so I have removed the event logic from the parser and added new logic to the tools. I have also removed the outputParser.destroy as we no longer need it and a reference to rawNode, I think from the rule view, that would not have worked remotely.

> But again, for V2.
> 
> ::: browser/devtools/shared/test/browser_outputparser.js
> @@ +27,5 @@
> > +  testParseCssProperty();
> > +}
> > +
> > +function testParseCssProperty() {
> > +  let frag = parser.parseCssProperty("color", "red", {
> 
> That test is fine, I would just add another one with a more complex property
> like : "border: 1px solid red" and assert that the color swatch does appear
> at the right position.
> 

I have now changed the simple property to your more complex one.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +2188,5 @@
> >        if (this.removeOnRevert) {
> >          this.remove();
> >        } else {
> > +        //this.prop.setValue(this.committed.value, this.committed.priority);
> > +        this.prop.setValue(this.valueSpan.textContent, this.committed.priority);
> 
> Nit: The commented out line of code should be removed.

Done

> And by the way, I
> would probably add a comment explaining why this.committed.value isn't used
> here as you explained.
> 

Done

> ::: toolkit/devtools/output-parser.js
> @@ +248,2 @@
> >      }
> > +    return false;
> 
> In my last review, when I said there should be a jsdoc comment, I was
> referring to the _appendColor function. It returns a boolean but the
> documentation doesn't say so and doesn't explain why it returns it.
> I would have personally preferred to keep the isColorValid function because
> it's an "isSomething" type of function so it's very obvious it returns a
> boolean, and I would have removed the return statements from _appendColor.
> But it's not a big deal either, so either way, I'm fine.

Yes, the problem is that we are not only checking for isValid, we also need to check for a special value so handling it using a color object makes more sense here.

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

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

Comment on attachment 830218 [details] [diff] [review]
color-swatch-missing-with-href-929384.patch

We need to uplift this in order to land bug 939098, which is already approved.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DevTools output parser
User impact if declined: Class names in markup panel may be replaced by color swatches e.g. class="red" would be displayed as class="#F00", class="inherit" would be displayed as class="black", CSS shortcut properties such as border: 1px solid red will not receive color swatches plus various other issues.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29331d8b292d

Comment 15

[u279076]

Given in-testsuite coverage this fix will not be manually verified by QA. If you believe this warrants extra QA attention please nominate for testing by removing this whiteboard tag and adding the verifyme keyword. Please also provide any details you have that may inform our testing.