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-cascade] About omitted subproperty value in shorthand #1068

Closed
upsuper opened this issue Mar 1, 2017 · 21 comments
Closed

[css-cascade] About omitted subproperty value in shorthand #1068

upsuper opened this issue Mar 1, 2017 · 21 comments

Comments

@upsuper
Copy link
Member

upsuper commented Mar 1, 2017

There is an implementation difference on what should happen for omitted subproperty value in shorthand.

Currently, Gecko and Edge reset missing subproperties in shorthand to their initial value as a declared value, while Blink and WebKit sets them to the initial keyword.

The spec says:

When values are omitted from a shorthand form, unless otherwise defined, each “missing” sub-property is assigned its initial value.

so it seems to me Gecko and Edge match what the spec says. But I can see that the text here can be easily misunderstood given the existence of initial keyword.

Can we add a note to the spec to emphasize that it is not resetting missing subproperties to the initial keyword? This may be worth adding a new CSSOM test into wpt.

I found this when I'm fixing Servo's code, which somehow largely followed the WebKit's way, and thus fails lots of tests in Gecko. It seems to me setting initial value to subproperties makes serialization code more complicated because individual shorthands would need to take CSS-wide keywords into consideration as well (and consequently lots of bugs in Servo serialization code in edge cases), so I don't want to adopt WebKit's approach here.

There is no known web-compat issue so far, but this is a detectable difference, so I also hope Blink and WebKit can fix this before any real web-compat issue appears.

@fantasai
Copy link
Collaborator

fantasai commented Mar 2, 2017

Um, how are you detecting this difference? Any difference here should be undetectable. https://www.w3.org/TR/css-cascade-3/#initial

@fantasai fantasai removed the Agenda+ label Mar 2, 2017
@upsuper
Copy link
Member Author

upsuper commented Mar 2, 2017

Easy: get the specified value via CSSOM, which is the value before cascading. e.g.

<!DOCTYPE html>
<div style="background: none"></div>
<script>
var elem = document.querySelector('div');
alert(elem.style.backgroundColor);
</script>

gives us

  • Firefox & Edge -> transparent
  • Chrome & Safari -> initial

Also, by "makes serialization code more complicated" in my first post, I meant I imagine that it makes serialization code potentially more complicated and error-prone in all browsers if follow that way, not just in Servo. At least, makes shorthand harder to round-trip.

For example, Blink has this kind of behavior that

<!DOCTYPE html>
<div style="background: none; background-size: 10px; background-position: initial;"></div>
<script>
var elem = document.querySelector('div');
alert(elem.style.backgroundPosition); // -> "initial"
elem.style.cssText = elem.style.cssText;
alert(elem.style.backgroundPosition); // -> "0% 0%"
</script>

And WebKit is somehow worse in this case that the second one returns empty directly. This kind of issues are fixable, but that just makes the logic unnecessarily complicated.

@upsuper
Copy link
Member Author

upsuper commented Mar 3, 2017

A probably more interesting one:

<!DOCTYPE html>
<div style="background: none, none"></div>
<script>
var elem = document.querySelector('div');
alert(elem.style.cssText);
elem.style.backgroundPosition = elem.style.backgroundPosition;
alert(elem.style.cssText);
elem.style.cssText = elem.style.cssText;
alert(elem.style.cssText);
</script>

This has different behavior between Chrome and Safari, and they both fail to round-trip in this case. Their three output of cssText are completely different.

@FremyCompany
Copy link
Contributor

I have been advocating internally for some time already we should probably match Chrome. This is more useful to developers than the current behavior. That being said, @dbaron has a general preference for the more backwards-compatible option when it comes to serialization, which would be the Edge/Firefox behavior. I would argue that if Chrome/Safari were able to switch we should be just fine doing so too, but at the same time switching isn't a big priority for us :-)

Do you have compat data that prompted you to ask, or is it just a question on what the new gecko/servo style engine should do?

@upsuper
Copy link
Member Author

upsuper commented Mar 3, 2017

It doesn't seem to me Chrome's behavior is more useful. It seems more buggy and unpredictable.

I agree that shorthand serialization omitting unspecified values probably has some value, but I would argue that it makes things more complicated than it should be. And simply omitting initial values in shorthand serialization when possible (rather than parsing omitted values as initial keywords and omitting those keywords in serialization) should be fine enough for most of cases (like what Edge has already been doing).

As I stated before, there is no compat issue so far. It is just a question raised when I was looking at Servo's code and realized why it was so buggy.

@FremyCompany
Copy link
Contributor

Oh I was not speaking about cssText serialization, but more that I like el.style.backgroundSize returning "initial" instead of "auto" because "auto" was never specified by the author; the property has its "initial| value instead, which happens to be auto. 

For shorthands I think what Edge does is more pleasant but requires written rules for every property and as far as I can remember the current proposal by Google (by shane I think) was to define more general rules but these were not ready last f2f so it hasn't been discussed extensively yet.

@upsuper
Copy link
Member Author

upsuper commented Mar 5, 2017

Oh I was not speaking about cssText serialization, but more that I like el.style.backgroundSize returning "initial" instead of "auto" because "auto" was never specified by the author; the property has its "initial| value instead, which happens to be auto.

As I mentioned before, making it return initial instead of initial value like auto makes serialization code unnecessarily more complicated and error-prone, and it doesn't even work in many cases. For background-size particularly, what should background: url(a) 100%, url(b); produce? background-size: 100%, initial? background-size:?

What would you expect from the following code to output?

<!DOCTYPE html>
<div id="ref" style="background: url(#a), url(#b)"></div>
<div id="test"></div>
<script>
var ref = document.getElementById('ref');
var test = document.getElementById('test');
alert(ref.style.backgroundSize);
test.style.backgroundSize = ref.style.backgroundSize;
alert(test.style.backgroundSize);
</script>

@FremyCompany
Copy link
Contributor

Lists form a poorly thought part of css. The fact you cannot "initial" a part of a list is annoying and could be fixed. That would make Chrome's return value valid and solve the problem you are mentioning.

@upsuper
Copy link
Member Author

upsuper commented Mar 6, 2017

That way, a lot more questions would be open, e.g. what about background: url(a) 100%, url(b); background-size: initial;? Should it be serialized to background: url(a), url(b);? Does the latter produce background-size: initial, initial or background-size: initial? Would background-size: initial be identical to e.g. background-size: initial, initial, initial?

Also, if we allow initial as part of a value but not all, should we also allow other CSS-wide keywords to be treated the same way? (WebKit still has an issue that inherit can accidentally appear in serialization of shorthands, which seems to have been fixed in Blink.) And would that apply to only lists or any property which can accept multiple tokens? What about tokens inside functions?

I didn't say it is not feasible to do that... I just wonder whether it's worth the effort to fix that in the hard way.

IIRC, @tabatkins mentioned that he was considering making it possible to cascade individual entry in a list separately, which may change the landscape of how this kind of things should behave.

@FremyCompany
Copy link
Contributor

Yep, tab and myself do agree for a long time now that list items should be able to cascade separately, which would mean that supporting css keywords for individual list entries would make a lot of sense. Just, as you mention, this has never been a priority enough to actually happen, but there are proposals on the table.

@upsuper
Copy link
Member Author

upsuper commented Mar 6, 2017

Making list entry an atomic value and allowing it to have CSS-wide keywords probably make sense. It is more tricky when it comes to other shorthand properties.

Anyway, I think the bottom line is that serialization and parsing should be able to round-trip, so we need to consider them simultaneously when we want to introduce significant change to either.

@tantek
Copy link
Member

tantek commented Mar 6, 2017

As this still seems unresolved, and I think the discussion above shows it's something that just needs (hopefully) a few minutes and a group resolution to move forward, I'm re-adding Agenda+.

@tantek tantek added the Agenda+ label Mar 6, 2017
@upsuper
Copy link
Member Author

upsuper commented Mar 6, 2017

And although it mainly affects lists, other shorthands are also affected, like font. font: 14px sans-serif assigns line-height: normal in all browsers, and adding line-height: initial makes Chrome generate empty string for font shorthand, but Safari generates font: 14px/initial sans-serif. Not quite consistent.

@tabatkins tabatkins removed the Agenda+ label Mar 6, 2017
@tabatkins
Copy link
Member

I'm removing the Agenda+; there's nothing for the group to discuss. The spec is clear; the initial keyword is not the initial value of any property, so serializing with it is simply violating the spec.

@upsuper Safari is just straight up broken in that "font" example - that's not a valid "font" shorthand at all.

@upsuper
Copy link
Member Author

upsuper commented Mar 7, 2017

I think the spec is clear, but it seems

  • Chrome and Safari do not agree with the spec, and
  • @FremyCompany tends to agree with Chrome and Safari although Edge currently follows what the spec says.

So it's probably at least worth a note in the spec to clarify this, and probably ask Blink and WebKit to change their behavior to follow the spec (or reversely), so that authors wouldn't start depending on unspeced behavior of Blink and WebKit in the future.

@dbaron
Copy link
Member

dbaron commented Mar 7, 2017

Could somebody file Blink and WebKit bugs?

@jetvillegas
Copy link

@jetvillegas
Copy link

I think this is the Webkit variant:
https://bugs.webkit.org/show_bug.cgi?id=82078

@upsuper
Copy link
Member Author

upsuper commented Mar 7, 2017

Thanks, jet. I think given both Blink and WebKit have an issue open from their developers for this issue, we can probably close this, since it seems they do agree with the specced behavior, and are just not active on fixing them.

@FremyCompany
Copy link
Contributor

So the specced behavior is that you use the initial value of the property and not initial, right? That should pretty much be what we do, I liked returning "intial" better but it is true that it just doesn't work well with the rest of css, the font example is pretty convincing.

@tabatkins
Copy link
Member

Yeah, per @upsuper's original post, the spec says:

When values are omitted from a shorthand form, unless otherwise defined, each “missing” sub-property is assigned its initial value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants