Closed Bug 1374513 Opened 7 years ago Closed 7 years ago

stylo: filter serialization is different

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hiro, Assigned: chenpighead)

References

(Blocks 1 open bug, )

Details

A test in dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html expects 
"blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6) saturate(0.3)"
Blocks: 1292283
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Hmmm, looks like I need to fix the value computing/storage here [1], and fix any servo layout failures caused by my changes as well.


[1] https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/values/specified/effects.rs#93-110
Priority: -- → P2
Here's a simple test case:
```
data:text/html, <img alt="Test_Form.jpg" id="img1" src="Test_Form.jpg" style="width: 300px; filter:opacity(30%);" />
```

The results of running "getComputedStyle(document.getElementById('img1')).filter" are:

opacity(0.3) in Chrome and Safari,
opacity(30%) in Firefox and Edge.


According to the latest reply from spec issue [1], Tabatkins seems lean on Chrome's behaviour as well. Although I'm not sure when will this issue be resolved and get specified.

So, instead of making a lot of changes to servo just for stylo, and taking the risk of we might revert all these changes in future, I'd like to try to make Firefox's behaviour match Chrome/Safari/Servo.



[1] https://github.com/w3c/csswg-drafts/issues/1546#issuecomment-314897594
[2] https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/values/specified/effects.rs#94
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> A test in
> dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html
> expects 
> "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6)
> saturate(0.3)"

I run the test with the latest m-c stylo build, and get the failure as follows:
```
assert_equals: value for 'filter' on ComputedKeyframe #1 expected "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(60.000004%) saturate(30.000002%)"
```

This looks more like a precision/rounding issue than a serialization issue now...

Hi Hiro, any idea what could be changed so that the failure you mentioned in comment 0 no longer exists? If you can confirm that comment 0 is now invalid, maybe we should duplicate this bug to Bug 1370779.
Flags: needinfo?(hikezoe)
See Also: → 1381232
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> Here's a simple test case:
> ```
> data:text/html, <img alt="Test_Form.jpg" id="img1" src="Test_Form.jpg"
> style="width: 300px; filter:opacity(30%);" />
> ```
> 
> The results of running
> "getComputedStyle(document.getElementById('img1')).filter" are:
> 
> opacity(0.3) in Chrome and Safari,
> opacity(30%) in Firefox and Edge.
> 
> 
> According to the latest reply from spec issue [1], Tabatkins seems lean on
> Chrome's behaviour as well. Although I'm not sure when will this issue be
> resolved and get specified.
> 
> So, instead of making a lot of changes to servo just for stylo, and taking
> the risk of we might revert all these changes in future, I'd like to try to
> make Firefox's behaviour match Chrome/Safari/Servo.
> 
> 
> 
> [1] https://github.com/w3c/csswg-drafts/issues/1546#issuecomment-314897594
> [2]
> https://searchfox.org/mozilla-central/rev/
> 152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/values/
> specified/effects.rs#94

Filed Bug 1381232 for this.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> > A test in
> > dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html
> > expects 
> > "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6)
> > saturate(0.3)"
> 
> I run the test with the latest m-c stylo build, and get the failure as
> follows:
> ```
> assert_equals: value for 'filter' on ComputedKeyframe #1 expected "blur(5px)
> sepia(60%) saturate(30%)" but got "blur(5px) sepia(60.000004%)
> saturate(30.000002%)"
> ```
> 
> This looks more like a precision/rounding issue than a serialization issue
> now...
> 
> Hi Hiro, any idea what could be changed so that the failure you mentioned in
> comment 0 no longer exists? If you can confirm that comment 0 is now
> invalid, maybe we should duplicate this bug to Bug 1370779.

Right, my guess is https://github.com/servo/servo/pull/17338 .
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(hikezoe)
Resolution: --- → FIXED
Running dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a Stylo build from yesterday's m-c, I get as one of the failures:

  assert_equals: value for 'filter' on ComputedKeyframe #0 expected "drop-shadow(10px 10px 10px rgb(0, 255, 0))" but got "drop-shadow(rgb(0, 255, 0) 10px 10px 10px)"

Is this expected?
Flags: needinfo?(jeremychen)
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #6)
> Running
> dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a
> Stylo build from yesterday's m-c, I get as one of the failures:
> 
>   assert_equals: value for 'filter' on ComputedKeyframe #0 expected
> "drop-shadow(10px 10px 10px rgb(0, 255, 0))" but got "drop-shadow(rgb(0,
> 255, 0) 10px 10px 10px)"
> 
> Is this expected?

I can confirm this in the latest autoland tip. Looks like a ordering issues for serialization.
I'll reopen this bug and take a look.
Status: RESOLVED → REOPENED
Flags: needinfo?(jeremychen)
Resolution: FIXED → ---
Thanks! If we decide that the Servo ordering is better (which might be the case if it matches the spec/Blink/etc.) then we should just file a bug on fixing Gecko (even if we never do), update the test, and mark the test as failing in Gecko (annotated with the bug to fix it).
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #8)
> Thanks! If we decide that the Servo ordering is better (which might be the
> case if it matches the spec/Blink/etc.) then we should just file a bug on
> fixing Gecko (even if we never do), update the test, and mark the test as
> failing in Gecko (annotated with the bug to fix it).

Hi Brian, sorry for the delay. Looks like the test is added in Bug 1374564, and we failed to catch this failure before landing because we haven't enable this test on try yet. I just saw this issue has been tracked in Bug 1377541, and there's patchset ready for review already, so I'm going to resolve this bug and let mantaroh fix the issue in Bug 1377541.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
No longer blocks: 1292283
Blocks: 1387080
You need to log in before you can comment on or make changes to this bug.