Skip to content

[css-properties-values-api] Syntax-checking in CSSStyleDeclaration.setProperty #778

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.

Sign up for GitHub

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

Jump to bottom
Closed
andruud opened this issue Jul 5, 2018 · 4 comments
Closed

[css-properties-values-api] Syntax-checking in CSSStyleDeclaration.setProperty #778

andruud opened this issue Jul 5, 2018 · 4 comments

Comments

@andruud
Copy link
Member

andruud commented Jul 5, 2018

In the current implementation of css-properties-values-api/cssom in Blink, it is not allowed to set a registered custom property to an invalid value using setProperty. The value is just discarded.

Should setProperty still discard invalid values for registered custom properties?

Example:

<!DOCTYPE html>
<script>
    CSS.registerProperty({
        name: '--color',
        syntax: '<color>',
        initialValue: 'green',
        inherits: false
    });
</script>
<style>div { --color: red; }</style>
<div id=target></div>
<script>
    // Has no effect:
    target.style.setProperty('--color', 'url("not-a-url")');

    // Actual: red
    // Expected (by andruud): green
    console.log(getComputedStyle(target).getPropertyValue('--color'));
</script>

This was allegedly [1] the agreed-upon behavior at some point, but as of 8dfc2d9 , this behavior seems inconsistent. I think it's more in line with the new spec wording to let setProperty ignore whether the property is registered or not, and do syntax-checking computed-value time.

[1] https://codereview.chromium.org/2607403002
@andruud
Copy link
Member Author

andruud commented Jul 11, 2018

The same goes for Typed OM, registering a property should have no effect on declared/inline style, but for computedStyleMap, getters return typed values after registering.

@andruud andruud mentioned this issue Jul 11, 2018
Closed
@andruud
Copy link
Member Author

andruud commented Jul 11, 2018 •
edited
Loading

.. or maybe not.

I guess it's kind of weird if, for a <length>-registered property --foo, this was invalid:

element.attributeStyleMap.set('--foo', CSS.px(20));

If typeness and syntax-checking really is a computed-value time thing only, here's how that could look in the spec: andruud@05ba373

@tabatkins
Copy link
Member

The behavior is inconsistent, but intentional.

Registered custom properties don't reject invalid values at parse time from a style sheet, so that browsers don't have to reparse all the styles on the page to determine the correct cascade (or, equivalently, preserve the entire list of cascaded values for every custom property); instead, you just decide at computed-value time whether the value that won the cascade is valid or not.

But there's no similar implementation difficulty for checking the validity of a style set from JS. Throwing an error gives useful feedback to the author, even if it does mean that there's now an order-dependence in the results.

(That is, setting a property then registering it gives no error, even if the value doesn't match the grammar; it just ends up as invalid at computed-value time. But registering the property and then setting it will throw an error if it doesn't match the grammar.)

I'll amend the note to make it clearer that this is an intentional inconsistency, however.

@andruud
Copy link
Member Author

andruud commented Jul 20, 2018

Gotcha. Thanks for that explanation.

The only thing I don't like about this, is that if a value is set with typed OM:

element.attributeStyleMap.set('--foo', CSS.px(20));

And we reference that value somewhere:

width: var(--foo);

Then we need to create a "synthetic" token stream for --foo for substitution purposes.

But I guess this is no worse than substituting an interpolated value. 🙂

@andruud andruud closed this as completed Jul 20, 2018
@andruud andruud mentioned this issue Feb 11, 2019
Closed
3 tasks
@emilio emilio mentioned this issue Apr 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@andruud @tabatkins