Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-typed-om] [css-paint-api] What happens to unsupported property names in the registered paint styleMap? #523

Closed
darrnshn opened this issue Nov 28, 2017 · 7 comments

Comments

@darrnshn
Copy link
Collaborator

@darrnshn darrnshn commented Nov 28, 2017

Suppose we specify unsupported property names calling registerPaint:

registerPaint('bleh', class {
    static get inputProperties() {
        return [
            'invalid', // invalid CSS property
            'border', // shorthand
        ];
    }
    paint(ctx, geom, styleMap) {
        const properties = styleMap.getProperties().sort();
        console.log(properties); // ??
   }
});

The paint spec says it's legal to have invalid properties in inputProperties() for forwards compat, but do these properties actually get returned by the style property map? Would the above output [] or ['invalid', 'border']? Note that normally (using just the Typed OM API), a StylePropertyMap can't contain unsupported property names.

@tabatkins
Copy link
Member

@tabatkins tabatkins commented Dec 12, 2017

Hmm, we do need to accept invalid properties, but reflecting them is problematic - what would they reflect as? The best we could do is treat them like untyped custom properties, and return a token list; this is weird tho, and seems just as likely to break naive code that assumes the property exists as just not returning the property at all in the map.

So, we should filter the list of input properties and store the filtered list as the set of listened-to properties that get reflected in the StylePropertyMap. That also matches up with the existing behavior of StylePropertyMap, as you cited.

@darrnshn
Copy link
Collaborator Author

@darrnshn darrnshn commented Jan 12, 2018

On a related note, what would be the behaviour of iterating over unregistered custom properties that were not specified? E.g. inputProperties has --bar but --bar is never referenced anywhere in the file, and then we get all property/value pairs in styleMap.

They're not "unsupported", e.g. styleMap.get("--bar") doesn't throw an error, but returns null. I see two options:

  1. Just ignore them like invalid properties.
  2. Return (--bar, null) as the entry.
@tabatkins
Copy link
Member

@tabatkins tabatkins commented Jan 12, 2018

Hm, they shouldn't return null, they should return an empty CSSUnparsedValue object, as that's the type for all unregistered properties.

@darrnshn
Copy link
Collaborator Author

@darrnshn darrnshn commented Jan 14, 2018

Oh interesting! Do you think we should clarify this in StylePropertyMap.get? Wording-wise, I guess we could either explicitly handle unregistered custom properties in the algorithm, or say that the property model implicitly contains an entry for every unregistered custom property?

@tabatkins
Copy link
Member

@tabatkins tabatkins commented Jan 16, 2018

Oooh, probably, yeah.

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Jan 19, 2018
Custom paint callbacks receive a computed style map (implemented as
FilteredComputedStylePropertyMap) that contain only properties specified
in inputProperties(). For forwards-compatiblity, inputProperties() is
allowed to have "invalid" or shorthand properties, which are not allowed
to be in the styleMap.

As resolved in [1], the style map should ignore any invalid/shorthand
properties. Furthermore, if a custom property is specified in
inputProperties(), it should have a default value of CSSUnparsedValue()
(not "null").

These changes also require changes in custom paint tests. Namely:

- Can no longer get "border-radius" from the style map. Have to use
  a longhand property like "border-left-radius".

- Getting custom properties like "--bar" will have a default value of
  CSSUnparsedValue.

[1] w3c/css-houdini-drafts#523

Bug: 803690
Change-Id: I148b52568de698f69ee0bf594a2b92a9751ea484
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Jan 20, 2018
Custom paint callbacks receive a computed style map (implemented as
FilteredComputedStylePropertyMap) that contain only properties specified
in inputProperties(). For forwards-compatiblity, inputProperties() is
allowed to have "invalid" or shorthand properties, which are not allowed
to be in the styleMap.

As resolved in [1], the style map should ignore any invalid/shorthand
properties. Furthermore, if a custom property is specified in
inputProperties(), it should have a default value of CSSUnparsedValue()
(not "null").

These changes also require changes in custom paint tests. Namely:

- Can no longer get "border-radius" from the style map. Have to use
  a longhand property like "border-left-radius".

- Getting custom properties like "--bar" will have a default value of
  CSSUnparsedValue.

[1] w3c/css-houdini-drafts#523

Bug: 803690
Change-Id: I148b52568de698f69ee0bf594a2b92a9751ea484
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Jan 20, 2018
Custom paint callbacks receive a computed style map (implemented as
FilteredComputedStylePropertyMap) that contain only properties specified
in inputProperties(). For forwards-compatiblity, inputProperties() is
allowed to have "invalid" or shorthand properties, which are not allowed
to be in the styleMap.

As resolved in [1], the style map should ignore any invalid/shorthand
properties. Furthermore, if a custom property is specified in
inputProperties(), it should have a default value of CSSUnparsedValue()
(not "null").

These changes also require changes in custom paint tests. Namely:

- Can no longer get "border-radius" from the style map. Have to use
  a longhand property like "border-left-radius".

- Getting custom properties like "--bar" will have a default value of
  CSSUnparsedValue.

[1] w3c/css-houdini-drafts#523

Bug: 803690
Change-Id: I148b52568de698f69ee0bf594a2b92a9751ea484
Reviewed-on: https://chromium-review.googlesource.com/874984
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530763}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Jan 23, 2018
Custom paint callbacks receive a computed style map (implemented as
FilteredComputedStylePropertyMap) that contain only properties specified
in inputProperties(). For forwards-compatiblity, inputProperties() is
allowed to have "invalid" or shorthand properties, which are not allowed
to be in the styleMap.

As resolved in [1], the style map should ignore any invalid/shorthand
properties. Furthermore, if a custom property is specified in
inputProperties(), it should have a default value of CSSUnparsedValue()
(not "null").

These changes also require changes in custom paint tests. Namely:

- Can no longer get "border-radius" from the style map. Have to use
  a longhand property like "border-left-radius".

- Getting custom properties like "--bar" will have a default value of
  CSSUnparsedValue.

[1] w3c/css-houdini-drafts#523

Bug: 803690
Change-Id: I148b52568de698f69ee0bf594a2b92a9751ea484
Reviewed-on: https://chromium-review.googlesource.com/874984
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530763}
@bfgeek
Copy link
Contributor

@bfgeek bfgeek commented Apr 8, 2018

@nainar @darrnshn What needs to happen in the Paint API spec for this?

@darrnshn
Copy link
Collaborator Author

@darrnshn darrnshn commented Apr 9, 2018

I think the paint API just needs to silently filter out invalid properties (CSS properties which are not implemented by that particular browser) from inputProperties (like foo). Not sure if the spec already mentions this.

@bfgeek bfgeek closed this in 50fa9b8 Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants