1397619 - stylo: serialization of specified value for some longhand properties is inconsistent

Status: RESOLVED FIXED

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

Description

[Daisuke Akatsuka (:daisuke)]

Attachment: serialization-specified-value.html

As like attached file, I tested by like following:

  element.style.setProperty('border-spacing', '10px');
  console.log(element.style.getPropertyValue('border-spacing'));

As result, for example, set '10px' to border-spacing, getPropertyValue returned '10px' as it is.
However, if set '10px' to border-top-left-radius, returned '10px 10px'.
According to Value item in the both specs, 'border-image-outset'[1] says '<​length​>{1,2}', 'border-top-left-radius'[2] says '<length-percentage>{1,2}'.

And another example, if specify 'underline' to text-decoration-line, returned value is 'underline'. But, if set 'markers' to paint-order, the value is 'markers fill stroke'. The specs are, 'none | [ underline || overline || line-through || blink ]' for text-decoration-line and 'normal | [ fill || stroke || markers ]' for paint-order.

I write the results as table with Servo, Gecko and Chrome result as below.

property	              |input value|       servo         |   gecko   |  chrome   | value spec
--------------------------------------------------------------------------------------------------------------------------------------------
border-spacing        |   10px    |        10px         |    10px   |   10px    | <​length​>{1,2}
border-top-left-radius|   10px    |      10px 10px      |    10px   |   10px    | <length-percentage>{1,2}
text-decoration-line  | underline |      underline      | underline | underline |'none  | [ underline || overline || line-through || blink ]'
paint-order           |  markers  | markers fill stroke	|  markers  |  markers  |'normal | [ fill || stroke || markers ]'

We may be able to ignore the difference since the spec in CSSOM[5, 6] does not say anything about serialization of this points.
However, I asked to Brian and he said, we should return minimal form basically. If so, border-top-left-radius should return '10px', paint-order should be 'markers' as same as Gecko and Chrome.

[1] https://drafts.csswg.org/css-tables/#propdef-border-spacing
[2] https://drafts.csswg.org/css-backgrounds-3/#border-top-left-radius
[3] https://drafts.csswg.org/css-text-decor-3/#text-decoration-line-property
[4] https://www.w3.org/TR/SVG2/painting.html#PaintOrderProperty
[5] https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue
[6] https://drafts.csswg.org/cssom/#serialize-a-css-value

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Servo PR to fix border-*-radius serialization: https://github.com/servo/servo/pull/18458

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: border-*-radius tests

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8907005 [details] [diff] [review]
border-*-radius tests

Review of attachment 8907005 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the tests, though the code changes need some more work.

::: layout/style/test/test_specified_value_serialization.html
@@ +256,5 @@
> +  var frame_container = document.getElementById("display");
> +  var p = document.createElement("p");
> +  frame_container.appendChild(p);
> +
> +  for (var i = 0; i < borderRadius.length; ++i) {

We may want to do this for all the border-radius longhands?

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

I am afraid that I won't be able to finish this one as I entered on parental leave yesterday.

The remaining work is to address Emilio's feedback on https://github.com/servo/servo/pull/18458 and fix the serialization of paint-order, which I didn't have time to investigate.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

No worries Fer, enjoy a lot!

I can take care of this one, I have a whole trans-atlantic flight to kill time on, and I just finished bug 1362532, so should be ok.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This will be fixed by https://github.com/servo/servo/pull/18525

Comment 7

[Bobby Holley (:bholley)]

This landed. Over to Emilio to push the tests.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> This landed. Over to Emilio to push the tests.

Specifically: https://hg.mozilla.org/integration/autoland/rev/446fa630c6ca

Comment 9

[Bobby Holley (:bholley)]

Downgrading the priority given that this bug is just about the tests now.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I just noticed that paint-order had gone unaddressed. I submitted https://github.com/servo/servo/pull/18627 for that. Will add the tests right after that lands.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e5c6748377b
stylo: test border-radius longhand specified values serialization. r=emilio
https://hg.mozilla.org/integration/autoland/rev/82b87c3aba50
Test border-spacing specified value serialization. r=me
https://hg.mozilla.org/integration/autoland/rev/9c7f4ff90f2a
Test paint-order specified value serialization. r=me

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e5c6748377b
https://hg.mozilla.org/mozilla-central/rev/82b87c3aba50
https://hg.mozilla.org/mozilla-central/rev/9c7f4ff90f2a

Comment 13

[Bobby Holley (:bholley)]

Request uplift?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8907005 [details] [diff] [review]
border-*-radius tests

This uplift request should include:

 * https://hg.mozilla.org/integration/autoland/rev/446fa630c6ca
 * https://hg.mozilla.org/integration/autoland/rev/72baa39ccf4c

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: potential web-compat issues.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: Just changes to CSS serialization to make it totally compatible with other browsers.
[String changes made/needed]: none

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8907005 [details] [diff] [review]
border-*-radius tests

Fix some potential webcompat issue.
Taking it in 57b4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
>  * https://hg.mozilla.org/integration/autoland/rev/446fa630c6ca

This landed before the final merge of 57 to Beta, so it's already there.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e4272e16bb2
https://hg.mozilla.org/releases/mozilla-beta/rev/dea074a3264f
https://hg.mozilla.org/releases/mozilla-beta/rev/772fb56b0982
https://hg.mozilla.org/releases/mozilla-beta/rev/9c121063b2ae

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: n/a
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.