Closed Bug 1367028 Opened 7 years ago Closed 7 years ago

Gecko serializes rectangular values (<top> <right> <bottom> <left>) differently than WebKit and Edge

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: nox, Assigned: xidorn)

References

Details

(Whiteboard: [stylo])

Attachments

(3 files)

Bug 1367028 part 1 - Simplify serialization of specified rect values in border-image props when possible.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1367028 part 2 - Simplify serialization of computed value of rect values in border-image props when possible.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1367028 part 3 - Add tests for the new serialization code.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
```
<div style='border-image: url("foo.png") 5 5 / 5 / 5 repeat repeat'></div>
<script>
    var div = document.getElementsByTagName("div")[0];
    console.log(div.style.borderImage);
    console.log(getComputedStyle(div).borderImage);
</script>
```

The "5 5 / 5 / 5" part gets serialised as "5 / 5 / 5" in Edge, WebKit, and Servo with https://github.com/servo/servo/pull/17002, but as "5 5 5 5 / 5 5 5 5 / 5 5 5 5" in Firefox.

Gecko should align its behaviour with the rest of the main user agents.
What does Stylo do?
Servo does "5 5 5 5" currently, and will do just "5" with the PR I mentioned above, aligning it with WebKit and Edge and making the code simpler.
Priority: -- → P2
Whiteboard: [stylo]
I don't change the serialization of computed value because that is a bit different and it is not required for shipping stylo.
Assignee: nobody → xidorn+moz
The serialisation of computed values is the same as what I described in all browsers but Gecko, AFAIK.
The fact that this shows up in the "border-image" shorthand property is a secondary effect here.  Really, the main issue here is really with the longhand properties:
  - border-image-slice
  - border-image-outset
  - border-image-width

You can reproduce this same issue (and discrepancy w/ other browsers) by simply setting any of those properties to "5" and then inspecting the serialized & computed style.

It looks like the patch fixes the serialized-style part of this (I think) but not the computed-style part -- I expect window.getComputedStyle(div, "").borderImageOutset will will return "5 5 5 5" with the attached patch, right?  (whereas in Chrome it'll return "5")

I think the fix here needs to:
 - Fix the computed style representation as well (to maintain consistency with ourselves & with other browers)
 - Test the longhand properties directly, perhaps with testcases in:
 https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_computed_style.html
 https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_specified_value_serialization.html
You should include testcases in those ^^ mochitests for the two-value serialization behavior as well -- e.g.:
  - test that "border-image-outset: 5 6 5 6" serializes to "5 6".
  - test that "border-image-outset: 5 5 6 6" serializes to itself since it doesn't have the pairs matched up correctly for the two-value syntax.
Comment on attachment 8872236 [details]
Bug 1367028 part 1 - Simplify serialization of specified rect values in border-image props when possible.

https://reviewboard.mozilla.org/r/143718/#review147630

r- for now per comment 6.  Also, one observation / possible-nit:

::: layout/style/test/test_shorthand_property_getters.html:236
(Diff revision 1)
>  e.setAttribute("style", 'border-image: url("foo.png") 5 5 5 5 / 5 5 5 5 / 5 5 5 5 repeat repeat; border-left: medium solid green');
>  is(e.style.cssText,
> -   'border-image: url("foo.png") 5 5 5 5 / 5 5 5 5 / 5 5 5 5 repeat repeat; border-left: medium solid green;',
> +   'border-image: url("foo.png") 5 / 5 / 5 repeat repeat; border-left: medium solid green;',

This piece of this mochitest isn't really *intending* to test/exercise this value-collapsing behavior (that "5 5 5 5" collapses to "5".)  Really, it's just intending to test that a setAttribute("style" call took effect.

So you should probably make the same change to the setAttribute call -- collapsing "5 5 5 5" quartets to just "5" -- to keep it consistent with the value that we're checking for here.  That'd make this test a little more understandable.

(It *is* important to test the 5 5 5 5 --> 5 collapsing behavior, but per comment 6, we should perform that check in a more dedicated test)
Attachment #8872236 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #7)
> You should include testcases in those ^^ mochitests for the two-value
> serialization behavior as well

nox points out in IRC that there's a three value syntax as well, e.g.:
  - "5 6 5" should serialize as "5 6"
  - "5 6 7" should serialize as "5 6 7"

It occurs to me that we already have this implemented for margin/padding/border-width, and it seems to work how I expect it should (abbreviating values as-possible).  Rather than fixing up the border-image serialization code to reimplement the abbreviation behavior, perhaps we should refactor that existing code into a helper function which we can share with border-image-* properties?

(The helper function would need to accept unitless values of course, which it probably doesn't right now for margin/border-width/padding.)
> It looks like the patch fixes the serialized-style part of this (I think) but not the computed-style part -- I expect window.getComputedStyle(div, "").borderImageOutset will will return "5 5 5 5" with the attached patch, right?  (whereas in Chrome it'll return "5")

Yes, that is intentionally as I mentioned in comment 4 that:
1. doing that serialization in computed style is a bit more annoying, and
2. it is not required for shipping stylo.

But okay, given both you and nox raised this issue, I should probably just fix that.
Looks like changing the serialization of computed value actually adds lots of failures to handle... (That's why I didn't really want to touch things which doesn't necessarily need to be changed... especially when it was good enough.)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> 2. [abbreviating computedsytle] is not required for shipping stylo.

Is it because Stylo isn't modifying our getComputedStyle implementation?  (Honest question -- I wasn't aware of this difference in priorities between our two paths for serialization. But I can imagine how this might be the case.)

> Looks like changing the serialization of computed value actually adds lots of failures to handle...

Try run, for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d5bd03eb9f4&selectedJob=102944024

The "failures" there don't look too nasty/numerous -- and they're all cases where we simply need to update the expectations, due to this legitimate change in getComputedStyle serialization behavior.

For example: in the wpt10 orange in animation "xyz-per-property" tests, it looks like a lot of failures but I'll bet it's sufficient to just change these 3 lines (in a common data file which these tests share)
https://dxr.mozilla.org/mozilla-central/rev/34ac1a5d6576d6775491c8a882710a1520551da6/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#193,205,219

> (That's why I didn't really want to touch things which doesn't
> necessarily need to be changed... especially when it was good enough.)

I understand, but our existing behavior for specified-style serialization is arguably just as "good enough" too. :)

My position is: right now, we (Gecko) are internally consistent w.r.t. how we serialize these properties, when comparing their specified style vs. computed style.  That consistency seems useful & desirable.  If we're changing from one arbitrary serialization to another in one of those codepaths, it seems like we should make that same change in both places, to preserve our current (desirable)internal consistency.
Stylo does this for:

* border-color
* border-style
* margin
* padding
* border-width
* inset()
* border-radius
* border-image-outset
* -moz-outline-radius
Forgot to repeat 2 that Daniel already mentioned:

* border-image-slice
* border-image-width
Comment on attachment 8872236 [details]
Bug 1367028 part 1 - Simplify serialization of specified rect values in border-image props when possible.

https://reviewboard.mozilla.org/r/143718/#review147994

r=me
Attachment #8872236 - Flags: review?(dholbert) → review+
Comment on attachment 8872506 [details]
Bug 1367028 part 2 - Simplify serialization of computed value of rect values in border-image props when possible.

https://reviewboard.mozilla.org/r/144042/#review147998

r=me
Attachment #8872506 - Flags: review?(dholbert) → review+
Comment on attachment 8872507 [details]
Bug 1367028 part 3 - Add tests for the new serialization code.

https://reviewboard.mozilla.org/r/144044/#review148004

r=me
Attachment #8872507 - Flags: review?(dholbert) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c472a9bfb4
part 1 - Simplify serialization of specified rect values in border-image props when possible. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2b57daca7dcb
part 2 - Simplify serialization of computed value of rect values in border-image props when possible. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a5a48d024030
part 3 - Add tests for the new serialization code. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/43c472a9bfb4
https://hg.mozilla.org/mozilla-central/rev/2b57daca7dcb
https://hg.mozilla.org/mozilla-central/rev/a5a48d024030
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: