Closed
Bug 1374513
Opened 7 years ago
Closed 7 years ago
stylo: filter serialization is different
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
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)"
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
(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)
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Reporter | ||
Comment 5•7 years ago
|
||
(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
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
(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 → ---
Comment 8•7 years ago
|
||
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).
Assignee | ||
Comment 9•7 years ago
|
||
(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 ago → 7 years ago
Resolution: --- → FIXED
Comment hidden (obsolete) |
You need to log in
before you can comment on or make changes to this bug.
Description
•