Closed Bug 1397619 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: daisuke, Assigned: emilio)

Details

Attachments

(2 files)

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
Priority: -- → P2
Assignee: nobody → ferjmoreno
Servo PR to fix border-*-radius serialization: https://github.com/servo/servo/pull/18458
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?
Attachment #8907005 - Flags: review?(emilio) → review+
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.
Assignee: ferjmoreno → nobody
Flags: needinfo?(emilio)
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
This landed. Over to Emilio to push the tests.
Assignee: nobody → emilio
(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
Downgrading the priority given that this bug is just about the tests now.
Priority: P2 → P4
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.
Priority: P4 → P2
Pushed by ecoal95@gmail.com:
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
Flags: needinfo?(emilio)
Request uplift?
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8907005 - Flags: approval-mozilla-beta?
Comment on attachment 8907005 [details] [diff] [review]
border-*-radius tests

Fix some potential webcompat issue.
Taking it in 57b4
Attachment #8907005 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.