Closed Bug 1268082 Opened 4 years ago Closed 4 years ago

replace inIDOMUtils.cssPropertySupportsType

Categories

(DevTools :: Framework, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 2 obsolete files)

For the devtools.html (track 3) project, we need to get rid of our usage of inIDOMUtils.cssPropertySupportsType in devtools client code.

It is currently used only in devtools\client\shared\output-parser.js to determine if a given CSS type is supported by a given css property name.

This might need to become an async thing that we ask the server to tell us.
We could simply do this in Javascript by creating a dummy DOM node, set style on it and get its computed style, but the answer is likely to depend on which backend we are connecting to, so it might be better to delegate this to the server.
While we're working on this, and if we end up using something else than DOMUtils, we might want to check that things like this do work:

DOMUtils.cssPropertySupportsType("filter", DOMUtils.TYPE_COLOR)

This currently return false, however a CSS filter can contain colors, for example:

drop-shadow(0px 0px 5px rgba(255, 0, 0, 0.5))
(In reply to Patrick Brosset <:pbro> from comment #1)
> While we're working on this, and if we end up using something else than
> DOMUtils, we might want to check that things like this do work:
> 
> DOMUtils.cssPropertySupportsType("filter", DOMUtils.TYPE_COLOR)
> 
> This currently return false, however a CSS filter can contain colors, for
> example:
> 
> drop-shadow(0px 0px 5px rgba(255, 0, 0, 0.5))

The contract of these functions is peculiar.
In this case I think the idea is that the filter itself can't take a color --
the drop-shadow function does.

It may be fine to change the contract to make more sense but this will require
extra testing.
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
One option to deal with this:

In bug 1265798, we're adding a new actor that sends a list of supported CSS property names to the toolbox.
I'm thinking we can base the work here on this bug, and just add the types that each property supports.

Looking at DOMUtils, it seems the CSS types are:
TYPE_ANGLE
TYPE_COLOR
TYPE_FREQUENCY
TYPE_GRADIENT
TYPE_IMAGE_RECT
TYPE_LENGTH
TYPE_NUMBER
TYPE_PERCENTAGE
TYPE_TIME
TYPE_TIMING_FUNCTION
TYPE_URL

So we could make the CSS database contain a dictionary with these types, and then have each property say which of these types it supports.

cssTypes: {TYPE_ANGLE: 1, TYPE_COLOR: 2, TYPE_FREQUENCY: 3, TYPE_GRADIENT: 4, TYPE_IMAGE_RECT: 5, TYPE_LENGTH: 6, TYPE_NUMBER: 7, TYPE_PERCENTAGE: 8, TYPE_TIME: 9, TYPE_TIMING_FUNCTION: 10, TYPE_URL: 11},
cssProperties: {
  align-content: [],
  background-color: [2],
  background-image: [2, 4, 5, 11]
  animation: [9, 10]
  ... and so on ...
}
This is just to illustrate comment 3, and is based on the patch in bug 1265798.
This exposes a new 'propertySupportsType' method on CssPropertiesFront that relies on data sent by CssPropertiesActor.
I'm starting to reconsider if we need this complexity at all.

We use this DOMUtils function in only one place (the output-parser) to do one thing only: check if timing functions or colors are accepted in given properties. We do this so that when we find strings that look like timing functions (i.e. linear) or like colors (i.e. red), then we generate swatches for those, so users can click on them to edit the value.

Now, timing-functions can only be found in 2 properties: transition and animation, and this is true in any browser.
And colors can be found in much more properties, but I feel like we could list these properties quite easily, across all browsers, and just maintain a static list on the client.

We do this only for one purpose: avoiding to mistake strings that look like timing functions or colors but that aren't. For instance `font-family: black;`. This is correct CSS and we don't want to mistake black for a color, because it isn't. Same with `foo: linear;`. foo isn't a valid property, and even if it was, linear isn't a valid value for it, so we don't want to generate a timing-function swatch here either.

So I feel like having a static list of properties that do accept these would work nicely.

I found one corner case, there might be more:
animation: linear 3s ease-in-out;
Here it looks like `linear` is the animation name, but browsers will consider the first name that looks like a timing-function to be the timing-function. So really, here, the animation name is `ease-in-out`. Again, I feel like this could be coded into the output-parser, without having to rely on something server-side.
For instance, in Firefox 49, here are the properties that accept colors:

-moz-border-bottom-colors
-moz-border-end
-moz-border-end-color
-moz-border-image
-moz-border-left-colors
-moz-border-right-colors
-moz-border-start
-moz-border-start-color
-moz-border-top-colors
-moz-column-rule-color
-webkit-border-image
-webkit-box-shadow
-webkit-filter
-webkit-text-fill-color
-webkit-text-stroke
-webkit-text-stroke-color
background
background-color
background-image
border
border-block-end
border-block-end-color
border-block-start
border-block-start-color
border-bottom
border-bottom-color
border-color
border-image
border-inline-end
border-inline-end-color
border-inline-start
border-inline-start-color
border-left
border-left-color
border-right
border-right-color
border-top
border-top-color
box-shadow
color
fill
filter
flood-color
lighting-color
outline
outline-color
stop-color
stroke
text-decoration
text-decoration-color
text-emphasis
text-emphasis-color
text-shadow
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8753901 - Attachment is obsolete: true
Comment on attachment 8757975 [details]
Bug 1268082 - Get the list of supported CSS types for each property from the server;

https://reviewboard.mozilla.org/r/56326/#review53234

I think the patch itself is fine.  I had one nit but it's trivia.

The bigger problem I see is with the plan.  I agree, the current database can be static and that's perfectly fine.  The issue is rather how to keep it up to date.  With the actor plan this is automatic -- changes to CSS properties in core are reflected through inIDOMUtils and then from the server to the client.  With the new plan, I imagine the idea has to be hoping that we notice whenever CSS changes are made.  To me this doesn't seem too likely; but I don't really know; maybe we have some good way to track such changes or maybe they just aren't very likely.

On the other hand I thought Greg's approach was to just ship the database of properties over at startup; and that adding a new query like supports-color would be easy to do this way.  So I think I don't understand why we'd prefer to avoid that -- the previous plan seemed low-cost and correct.

I'm happy to r+ this but I would like to understand this part first.

::: devtools/client/shared/css-properties-db.js:439
(Diff revision 1)
> +  "-moz-transition", "-moz-transition-timing-function", "-webkit-transition",
> +  "-webkit-transition-timing-function"
> +];
> +
> +// Properties that accept colors.
> +exports.COLOR_PROPERTIES = [

I think this comment should explain that this list also holds any property that can accept a gradient.
Attachment #8757975 - Flags: review?(ttromey)
The rationale was:
- The list of css properties that accept values that may contain colors is rather small.
- This list doesn't change very often either (I think the last new property that was added that accepts colors was "filter", and it was added 1,5 year ago, and I'm not aware of any new CSS properties that accept colors being added now).
- Of course we could miss new properties but the fix would be trivial (good first bug).
- Having this list allows us to easily fix cases where filter isn't recognized as supporting colors by DOMUtils.cssPropertySupportsType. All we want to know is if for a given name, we may find colors in the value. That static list of property names gives us that.
- Introducing, for each property, the list of supported types to Greg's css-property actor seemed more complex in comparison, with little added benefit. This would make the packet bigger (though performance shouldn't be too much of a problem here since we send it only once), and would contain other css types that we don't need (we'd have to send, for each and every property, an array of types, as strings).
- For backward compatibility reasons, we're going to need this list anyway (if you connect to an older server that doesn't have the css-property actor, then you need to get the data from somewhere else. Greg has introduced css-property-db.js for this purpose.

All of this made me think that it might just be simpler to have this hard-coded list client-side.
I saw very little benefit to getting the full list of supported types from the server.
And, in fact, I thought that managing this purely client-side could even help us in some cases. Like filter, mentioned above, but also why not display color-swatches next to invalid properties, if we know the property is just using an invalid vendor prefix for instance, or something like that.

I hope this makes my choice clearer. Let me know what you think. I can be convinced otherwise, but right now, I think the benefits outweigh the drawbacks.
(In reply to Patrick Brosset <:pbro> from comment #10)
> The rationale was:
[...]
> All of this made me think that it might just be simpler to have this
> hard-coded list client-side.

> I hope this makes my choice clearer. Let me know what you think. I can be
> convinced otherwise, but right now, I think the benefits outweigh the
> drawbacks.

What's the difference between the rationale for this case and for the other
cases, where we are sending data from the server?  Reading the list, it seems
to me that the rationale applies equally well to all the other methods.  So
why shouldn't we just put it all on the client and not bother with a new actor?
(In reply to Tom Tromey :tromey from comment #11) 
> What's the difference between the rationale for this case and for the other
> cases, where we are sending data from the server?  Reading the list, it seems
> to me that the rationale applies equally well to all the other methods.  So
> why shouldn't we just put it all on the client and not bother with a new
> actor?
Well, I think between browsers, the list of supported properties varies a lot (not all browsers implement all css properties, some have vendor prefixes, ...).
However, the list of properties that accept colors doesn't vary as much, if at all.
I think for the inspector it's safe to assume that if, say, filter is implemented, it accepts colors. Or, say, box-shadow isn't going to support colors in one browser and not in another one.
And new properties are added at a much higher rate than only color-accepting properties (for instance css grid layout, which is one of the most recent features, comes with ~15 new properties).
Comment on attachment 8757975 [details]
Bug 1268082 - Get the list of supported CSS types for each property from the server;

https://reviewboard.mozilla.org/r/56326/#review53622

Thanks for your explanation.  I'm still not 100% in agreement but I don't disagree enough to block this.
Attachment #8757975 - Flags: review+
Thanks Tom. I'd like Greg's opinion on this too. That might help take a decision one way or the other.
Flags: needinfo?(gtatum)
I think asking questions to a server is a more maintainable option than attempting to maintain a static list. Adding a property to a static list is an easy bug to fix, but could add more churn to the code base and maintenance work. Since the CssPropertiesActor architecture is in place, I don't feel that it adds much relative complexity to the code to implement this using the actor. This way we can totally rely on the server to tell us what it supports.

I think a good question to ask is whether or not the Chrome Debugging Protocol can support this question. I haven't looked much at the protocol yet to see if this is the case.
Flags: needinfo?(gtatum)
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #16)
> I think a good question to ask is whether or not the Chrome Debugging
> Protocol can support this question. I haven't looked much at the protocol
> yet to see if this is the case.
No it does not support this. Chrome devtools seem to do what I'm doing in the attached patch, that is they have a client-side list of properties that support colors: 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js&q=_colorAwareProperties&sq=package:chromium&type=cs&l=178
In fact, they don't even get their list of supported properties from the server. Everything is in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
That's what they use for autocompletion in the rule-view for instance.
Of course they don't support remote debugging of non-chrome browsers so it's much safer to depend on a client-side db than it is for us. But I was looking at the git log on that file and didn't notice too many updates related to adding new properties.

Tom and Greg have made good points, and I think I'll go with their suggestion. However, we need to keep in mind that the Chrome protocol doesn't support this. So that means if/when we connect to other browsers, we'll need to have that list either available on the client (css-properties-db) or have something like valence that returns a static list too.
I think I agree with :tromey and :gregtatum that asking the server seems more maintainable.  But :pbro is also correct that we'll still have an issue for other debugging targets like Chrome.

At least for Firefox, I think we'd want to ask the server so we can be sure to have an up to date list.  In general it's best for us to ask the server, esp. because of the way we handle backward compatibility for older Firefox.  But at the same time, we would want something for Firefox servers before the request to server was added, and also for other engines, so it seems like you want some static list as a fallback.

So, I guess I am arguing for "both". :) Hopefully that's not too complex here, I haven't looked at the patch.
Sorry about the delay here, I was working on another bug that took longer than expected. I have resumed working on this bug today, and will go with what Ryan said. Anyway, we need the types to be known in our fallback css-properties-db, so I'll have the server send them, but use the fallback for backward compatibility reasons. And when we connect to other browsers, then we'll be able to use it too.

Backward compatibility is now a bit more complex because the cssProperties actor has landed in 49, and we're now in 50. So not only do we have to check if the actor exists, but also which version of data it is sending.
In my case, if I connect to a FF49, I will get the list of properties from the server, but not the list of supported types. So I'll have to get those from the static list fallback.
Ok, this should work nicely. I'm going to work on it some more, and make sure tests pass before asking for review.
Attachment #8757975 - Attachment is obsolete: true
Comment on attachment 8757975 [details]
Bug 1268082 - Get the list of supported CSS types for each property from the server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56326/diff/1-2/
Attachment #8757975 - Attachment description: MozReview Request: Bug 1268082 - Add a list of color and timing-function taking properties to remove DOMUtils.cssPropertySupportsType; r=tromey → Bug 1268082 - Get the list of supported CSS types for each property from the server;
Attachment #8757975 - Attachment is obsolete: false
Attachment #8762003 - Attachment is obsolete: true
Comment on attachment 8757975 [details]
Bug 1268082 - Get the list of supported CSS types for each property from the server;

Damn, mozreview R+'d this automatically because of the old patch I had attached before.
Attachment #8757975 - Flags: review+ → review?(ttromey)
Comment on attachment 8757975 [details]
Bug 1268082 - Get the list of supported CSS types for each property from the server;

https://reviewboard.mozilla.org/r/56326/#review55882

Thank you.  This looks good to me.  One optional comment.

::: devtools/shared/css-properties-db.js:1627
(Diff revision 2)
> -    isInherited: false
> +    isInherited: false,
> +    supports: [10]
>    },
>    "-webkit-border-radius": {
> -    isInherited: false
> +    isInherited: false,
> +    supports: [6, 8]

Are we concerned about memory use storing all these arrays?  It would be more space efficient, and no worse, to make these bitfields, like `(1<<6)|(1<<8)`.

Feel free to ignore if this is of no concern.

::: devtools/shared/fronts/css-properties.js:88
(Diff revision 2)
> +   * Checks to see if the property is an inherited one.
> +   *
> +   * @param {String} property The property name to be checked.
> +   * @return {Boolean}
> +   */
>    isInherited(property) {

Thanks for adding this comment.
Attachment #8757975 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #24)
> Comment on attachment 8757975 [details]
> Bug 1268082 - Get the list of supported CSS types for each property from the
> server;
> 
> https://reviewboard.mozilla.org/r/56326/#review55882
> 
> Thank you.  This looks good to me.  One optional comment.
> 
> ::: devtools/shared/css-properties-db.js:1627
> (Diff revision 2)
> > -    isInherited: false
> > +    isInherited: false,
> > +    supports: [10]
> >    },
> >    "-webkit-border-radius": {
> > -    isInherited: false
> > +    isInherited: false,
> > +    supports: [6, 8]
> 
> Are we concerned about memory use storing all these arrays?  It would be
> more space efficient, and no worse, to make these bitfields, like
> `(1<<6)|(1<<8)`.
> 
> Feel free to ignore if this is of no concern.
I wasn't sure if we should be concerned about this, and had a quick conversation with Ryan about it. I don't think that's much of a problem. If we were constantly exchanging data over the protocol, it would be more of a problem, but that packet gets sent only once. And in terms of pure memory used to store this, this seems like a pretty small amount of data. There's like 300 or 400 properties, so I'm really not concerned about this.
Thanks for the review.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/bac29d6a5639
Get the list of supported CSS types for each property from the server; r=tromey
https://hg.mozilla.org/mozilla-central/rev/bac29d6a5639
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.1
Priority: P2 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.