1575062 - Add use counters for unimplemented CSS properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Boris Chiou [:boris]]

Given the list of CSS things that Chrome implements that we don't, we would like to add use counters for these properties.

Possible ways: Just try to parse them or piggy-back from error-reporting. Need to check the list for more information.

Comment 1

[Boris Chiou [:boris]]

The current version of the list from tdsmith: CSS properties implemented by Chrome 76, missing from Firefox 69.0b14

Comment 2

[Boris Chiou [:boris]]

Hi Emilio,

Could you please give me some advices for this bug. :)

First, we have to define a new pref for this, and this should be easy.

Second, assume we are using " try to parse them" for those unimplemented properties. so perhaps we can split it into 2 steps:

1. If there is a parse error when we are parsing a property id in PropertyDeclarationParser [1], we can try to parse the name again by the property list in comment 1. This means we need to store the special list in a place. (Could generate the enum with these keywords by mako I guess). If this name is in the list, we record it into self.context.use_counters (assume we enable layout.css.use-counters.enabled).

   • Or is there any better way to reduce the overhead to write the parsing code in [1]? For normal property, we define a property by the macro in xxx.mako.rs, and then the macro will generate the parsing code to parse the property name. So we probably could define a simplified macro to generate the parsing code for the special list, and put the special list into something like unsupporedOrUnshipped.mako.rs? The generated parse code will record the info into use_counters. I don't have a good idea for now.

2. We have to update the use_counters to support the list. That means NonCustomPropertyIdSet should include the unimplemented property id. It seems we also have to define the mapping between NonCustomPropertyId [2] and nsCSSPropertyID. However, I'm still looking at this because I'm not sure how to add a special list into nsCSSPropertyID for now. If you have any advice, please let me know. :)

[1] https://searchfox.org/mozilla-central/rev/03853a6e87c4a9405fce1de49e5d03b9e7a7a274/servo/components/style/properties/declaration_block.rs#1302-1306
[2] https://searchfox.org/mozilla-central/rev/03853a6e87c4a9405fce1de49e5d03b9e7a7a274/servo/components/style/properties/properties.mako.rs#424

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So there are a few key differences between this bug and the existing error reporting code:

• This is going to be enabled by default, in all docshells. So it should be as fast as possible.
• This doesn't need the property name as a string, or dealing with arbitrary unknown properties, as the error reporting does. We have a very well defined set of known properties.

So the way I had envisioned this to work without being a pain (which is what you're describing in (2), where suddenly we start having unsupported properties in NonCustomPropertyId and nsCSSPropertyID which we need to teach everyone not to handle or assert against or not) is something like the following:

• Define the list of properties somewhere in the python code. Somewhere in data.py or mako directly may do.
• Define a CountedUnknownProperty enum with variants for each property.
• Teach the phf-map in parse_unchecked to lookup these properties. This includes creating a different value type for that hashmap (we used to do that, actually, for a different reason, in bug 1499778). So the value for that hashmap should be something like:

enum StaticId {
    Longhand(LonghanId),
    Shorthand(ShorthandId),
    LonghandAlias(LonghandId, AliasId),
    ShorthandAlias(ShorthandId, AliasId),
    CountedUnknown(CountedUnknownProperty),
}

• Either (not sure what's cleaner yet) pass an Option<&ParserContext> (or Option<&UseCounters>) to parse_unchecked, which would do the use-counting, or return Result<PropertyId, Option<CountedUnknownProperty>> from it and let the callers do it. I think the first are going to be nicer. Passing just Option<&UseCounters> allows us to count from CSSOM very easily if we want to.
• Add an counted_unknown_properties field to UseCounters. This would be very similar to the existing NonCustomPropertyUseCounters but with the new enum rather than NonCustomPropertyId.
• Then the code in parse_unchecked would do something like the following:

if let Some(id) = property_id {
    return Ok(match id {
        StaticId::Longhand(longhand_id) => PropertyId::Longhand(longhand_id),
        // ...
        StaticId::CountedUnknown(unknown_id) => {
            if let Some(counters) = context.and_then(|c| c.use_counters.as_ref())  {
                counters.counted_unknown_properties.record(unknown_id);
            }
            return Err(()); // We know these aren't valid custom property names.
        }
    });
}

With an scheme like this there's no extra complexity in arbitrary places of the code (like new NonCustomPropertyIds that don't appear everywhere), and it seems to me that it'd be straight-forward.

We may need some little extra code to add tests for it and such, but I think something like that keeps the performance impact to the minimum, wdyt?

We may also want to consider to move non-custom-property counters also to the same place while we're at it, though it's harder to count CSSOM setters then. Maybe only if we do want to count them for unknown properties, see below.

Some other questions, maybe they matter maybe don't:

• Do we want to count CSSOM setters? (like people doing document.documentElement.style.setProperty("zoom", "foo")) or what not? Counting that seems straight-forward (just ensure we can pass use counters to PropertyId::parse_enabled_for_all_content, which is what we use for CSSOM). But counting the (probably more common) document.documentElement.style.zoom = "foo" case probably requires a bit more work in the WebIDL stuff.

• Do we want to exclude counting @supports or such? I don't think it's a big deal either way, but may be nice to explicitly consider it if it's not hard to implement.

I may have missed something else.

Boris, does the comment above help? Does it make sense to you? If not I'm happy to talk or video-meet or what not :)

Comment 4

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

We may need some little extra code to add tests for it and such, but I think something like that keeps the performance impact to the minimum, wdyt?

Yap. Putting all parsing code together in the same pattern match makes sense to me to minimize the impact.

Boris, does the comment above help? Does it make sense to you? If not I'm happy to talk or video-meet or what not :)

It's awesome and really helpful. Now I have a better concept for the implementation detail. Will let you know if I meet new problems. Thanks a lot! :)

Comment 5

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Some other questions, maybe they matter maybe don't:

• Do we want to count CSSOM setters? (like people doing document.documentElement.style.setProperty("zoom", "foo")) or what not? Counting that seems straight-forward (just ensure we can pass use counters to PropertyId::parse_enabled_for_all_content, which is what we use for CSSOM). But counting the (probably more common) document.documentElement.style.zoom = "foo" case probably requires a bit more work in the WebIDL stuff.

About CSSOM setters, perhaps this could be the 2nd step or a follow-up bug, I think.

• Do we want to exclude counting @supports or such? I don't think it's a big deal either way, but may be nice to explicitly consider it if it's not hard to implement.

Yap, could be a follow-up bug too.

Hi, Tim,

What do you think about the CSSOM setters and @support? For now, supporting CSS use counter for unimplemented properties on style sheet loading should be straight-forward, so I am working on this now first. About CSSOM setters and @support (or others), do you want to also count them right now, or it's fine to do them in the next step? (I can file a follow-up bug for them if we want to do that.) Thanks.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Yeah, fixing up both in follow-ups as needed seems better to me as well :)

Comment 7

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Do you think that using CSSOM that way is popular? Mike might have a better idea. Unless we know it's widely deployed, a followup bug sounds right.

Counting feature queries sounds like a nice-to-have; I don't think we need them in the first pass.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #7)

Do you think that using CSSOM that way is popular? Mike might have a better idea. Unless we know it's widely deployed, a followup bug sounds right.

I suspect using CSSOM without using it on stylesheets would be uncommon, but not unheard of.

Counting feature queries sounds like a nice-to-have; I don't think we need them in the first pass.

Oh I thought the goal was not to count feature queries? If you're feature-querying probably the feature doesn't give us compat headache. But IDK.

Comment 9

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #7)

Counting feature queries sounds like a nice-to-have; I don't think we need them in the first pass.

Oh I thought the goal was not to count feature queries? If you're feature-querying probably the feature doesn't give us compat headache. But IDK.

Ah, I didn't read closely enough; I thought Boris was proposing that they should be counted. Apologies!

I agree that excluding feature queries from the use counters is best, because they are not likely to produce a compat problem.

Pushing the work to exclude feature queries from the use counters to a follow-up bug is fine with me, if it adds complexity.

Feature queries are sort of interesting, in the sense that it's signal for a feature that developers are interested in deploying, but learning about those is not our primary objective.

Comment 10

[Mike Taylor [:miketaylr]]

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #7)

Unless we know it's widely deployed, a followup bug sounds right.

Follow up also sounds fine. I think we'll eventually want setters counted with the popularity of CSS-in-JS stuff (some of which just set inline styles), but I wouldn't block on it.

Comment 11

[Boris Chiou [:boris]]

Just get a question. For now, there are 3 kinds of properties:

1. Shipped CSS properties, e.g. width
2. No-shipped-yet CSS properties (i.e. developing CSS properties), e.g. column-span, offset-path, -webkit-app-region
3. Unimplemented CSS properties, e.g. zoom

The current implementation is for (1) and (2) (pref: layout.css.use-counters.enabled), and I'm working on (3). However, the list in comment 1 also includes (2). That means the phf-map in parse_unchecked() has duplicate entries because of (2). So I'd like to remove (2) from the list in comment 1, and do some trick things, based on our decision.

So now, I would like to make sure the purpose in this: do we want to record (2) in this bug? (or even together with (1))

• If (1) and (2) and (3) all should be recorded, we perhaps only need one preference (i.e. layout.css.use-counters.enabled), and the code might be simpler.
• if we just record (2) and (3), this may be a little bit tricky, and I need to maintain an extra list for (2).
• if only (3), this is trivial, I think.

May need Tim or Emilio's feedback. Thanks.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Chiou [:boris] from comment #11)

2. No-shipped-yet CSS properties (i.e. developing CSS properties), e.g. column-span, offset-path, -webkit-app-region

I guess -webkit-app-region was meant to be in (3)?

Anyhow, so that is indeed slightly annoying. I guess ideally we'd do something with (2) like: count it as (1) if enabled, as (3) otherwise.

That's doable, with some work I suspect. We could add the counting for those somewhere later like in allowed_in / enabled_for_all_content. Though it is kind of annoying implementation-wise. Do we really need data for properties we're already working on implementing?

In any case this work looks like it could be a separate bug once the unimplemented properties are done.

Comment 13

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

I guess -webkit-app-region was meant to be in (3)?

My typo. sorry. It's belonging to (3)

Anyhow, so that is indeed slightly annoying. I guess ideally we'd do something with (2) like: count it as (1) if enabled, as (3) otherwise.

This makes sense to me. I'd try this way in the following bug.

That's doable, with some work I suspect. We could add the counting for those somewhere later like in allowed_in / enabled_for_all_content. Though it is kind of annoying implementation-wise. Do we really need data for properties we're already working on implementing?

Perhaps before shipping? Just like to analyze how many documents use individual transforms, so we can prioritize it and decide the time to ship it, I guess.

In any case this work looks like it could be a separate bug once the unimplemented properties are done.

Yes. at least I will use a separate patch for these properties.

Comment 14

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Do we really need data for properties we're already working on implementing?

If we have something behind an off-by-default flag, then yeah, from a measurement perspective, I think we don't want to give ourselves credit before we deserve it.

It happens pretty often that features hit release in this state, right? If it was something that only happened in nightly or beta, I wouldn't worry about it.

In any case this work looks like it could be a separate bug once the unimplemented properties are done.

That works for me.

Comment 15

[Boris Chiou [:boris]]

Attachment: Bug 1575062 - Support css use counters for unimplemented properties.

Comment 16

[Boris Chiou [:boris]]

I am planning to land this in this week. The pref is off on beta/release, so should be safe.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f529796a87c2
Support css use counters for unimplemented properties. r=emilio

Comment 18

[Stefan Hindli [:stefan_hindli]]

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

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f529796a87c2