Closed
Bug 1451724
Opened 6 years ago
Closed 6 years ago
[stylo] CSS transforms margin-bottom and translate incorrect behavior
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: ggriffiths, Assigned: manishearth)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
2.28 MB,
image/gif
|
Details | |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
jcristau
:
approval-mozilla-esr60+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36 Steps to reproduce: See the attached GIF and Code below. - Applying a transform: translate on a child element, with a transition on the transform and applying a margin to the parent element, that also has transition on "margin-bottom", causes strange animation behavior in 60.0b9. (See in animated gif). Important things to note: prior to translate the child transform is: transform: translate(450px); after translate the child transform is: transform: translate(0px,0px); Note the omitting of the "Y-axis" transform in the first, and its presence in the second. If either: you add ",0px" to the first translate: issue is gone. you remove ",0px" (y-axis) from the second translate Y issue is gone. According to the spec: "specifies a 2D translation by the vector [tx, ty], where tx is the first translation-value parameter and ty is the optional second translation-value parameter. If <ty> is not provided, ty has zero as a value." if i set: transform: translate(0,0); Without the "px", issue is there still. Code to reproduce in the following HTML file: --- code start <!DOCTYPE html> <html> <style> .margin-anim { transition: margin-bottom 0.8s ease-in-out; } .rectangle { background:red; width: 356px; height: 48px; /* translate only specifying x */ transform: translate(450px); transition: transform 300ms; } .move-left { transform: translate(0px,0px); } .animate-margin-bottom { margin-bottom: 8px; } </style> <body> <div class="margin-anim"> <div class="rectangle"></div> </div> <script> function transform() { var rectangle = document.querySelector('.rectangle'); setTimeout(()=>{ // add class that defines margin on parent which is animated rectangle.parentElement.classList.add('animate-margin-bottom'); // transform child rectangle.classList.add('move-left'); },300); } function reset() { var rectangle = document.querySelector('.rectangle'); rectangle.parentElement.classList.remove('animate-margin-bottom'); rectangle.classList.remove('move-left'); } </script> <button id="transform" onclick="transform()">Transform</button> <button id="transform" onclick="reset()">Reset</button> </body> </html> --- code end Actual results: The animation goes in a "V" shape instead of side to side. I've tested in FF 52 ESR - issue is not present tested in Latest Chrome - issue is not present test in Edge - issue not present Expected results: Animation should function from right to left.
Comment 1•6 years ago
|
||
#1 regression, The rectangle is moved from right to left. But no transition animation: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6bc39f03ea571fb109ae603b0c8f69778ec14384&tochange=92ff0c88e94d176ff9615abe61322413b7f38e55 #1 Regressed by: 92ff0c88e94d Manish Goregaokar — servo: Merge #18750 - Make transforms generic (from Manishearth:transform-generic); r=emilio,xidorn #2 regression, The animation goes in a "V" shape instead of side to side: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=251fb7b394132abe8947b526def1318a59fd65e3&tochange=ba242fe9760e1744c2bdb463bbc679a741254cd5 #2 Regressed by: ba242fe9760e Manish Goregaokar — servo: Merge #19604 - stylo: Correctly handle interpolation where optional second argument for translate(), skew(), scale() exists in one but not the other (from Manishearth:animate-second); r=emilio
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
Ever confirmed: true
Keywords: regression
Summary: CSS transforms margin-bottom and translate incorrect behavior → [stylo] CSS transforms margin-bottom and translate incorrect behavior
Version: 60 Branch → 58 Branch
Comment 2•6 years ago
|
||
btw, no bugzilla #, It cannot block.... :(
Comment 3•6 years ago
|
||
@Manish Goregaokar, Your bunch of patch seems to cause the problem. Can you look into this?
Flags: needinfo?(manishearth)
Assignee | ||
Comment 4•6 years ago
|
||
I have to head out, but this is likely fixed by diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index bb6931cdde..342f0cb839 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1204,7 +1204,8 @@ impl Animate for ComputedTransformOperation { ) => { Ok(TransformOperation::Translate( fx.animate(tx, procedure)?, - Some(fy.unwrap_or(*fx).animate(&ty.unwrap_or(*tx), procedure)?) + Some(fy.unwrap_or(LengthOrPercentage::zero()) + .animate(&ty.unwrap_or(LengthOrPercentage::zero()), procedure)?) )) }, (
Assignee | ||
Comment 5•6 years ago
|
||
I don't quite see how the margin-bottom is relevant here, but I'll have a closer look
Flags: needinfo?(manishearth)
Comment 6•6 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #5) > I don't quite see how the margin-bottom is relevant here, but I'll have a > closer look Hiro may know, but I suspect the parent margin animation prevents the child animation from running on the compositor, and we use different code paths there... Or something like that.
Flags: needinfo?(hikezoe)
Comment 7•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6) > (In reply to Manish Goregaokar [:manishearth] from comment #5) > > I don't quite see how the margin-bottom is relevant here, but I'll have a > > closer look > > Hiro may know, but I suspect the parent margin animation prevents the child > animation from running on the compositor, and we use different code paths > there... Or something like that. That's quite right. We don't run transform animations on the compositor if there is other animation that affect geometric change (e.g. width, height, margin of course) and the animation started at the same time when the transform animations started.
Flags: needinfo?(hikezoe)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → manishearth
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8965710 [details] Bug 1451724: Add reftest ; https://reviewboard.mozilla.org/r/234238/#review240190 Looks good, thanks :)
Attachment #8965710 -
Flags: review?(emilio) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8965711 [details] Bug 1451724: Add reftest ; https://reviewboard.mozilla.org/r/234240/#review240196 There's no reason not to make this a WPT test, right? If so, let's do that instead. ::: layout/reftests/bugs/1451724-1-ref.html:15 (Diff revision 1) > +} > +</style> > +</head> > +<body> > +<div class=box id=box></div> > +<script type="text/javascript"> Same remark as in the other file. Also, please make this a WPT test instead. ::: layout/reftests/bugs/1451724-1.html:13 (Diff revision 1) > + height: 200px; > +} > +</style> > +</head> > +<body> > +<div class=box id=box></div> no need for both id and class, just use one. ::: layout/reftests/bugs/1451724-1.html:14 (Diff revision 1) > +} > +</style> > +</head> > +<body> > +<div class=box id=box></div> > +<script type="text/javascript"> no need for `type=`. ::: layout/reftests/bugs/1451724-1.html:15 (Diff revision 1) > +</style> > +</head> > +<body> > +<div class=box id=box></div> > +<script type="text/javascript"> > + let anim = document.getElementById('box').animate([ If you use id, you can just do `box.animate(..)`.
Attachment #8965711 -
Flags: review?(emilio)
Assignee | ||
Comment 12•6 years ago
|
||
fix landed as https://github.com/servo/servo/pull/20575, will push test soon
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965711 -
Attachment is obsolete: true
Comment 14•6 years ago
|
||
This merged.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 15•6 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #14) > This merged. For posterity: https://hg.mozilla.org/mozilla-central/rev/03d22e687292 Does this need the test to be pushed still? Also, does this need a Beta approval request?
Flags: needinfo?(manishearth)
Flags: in-testsuite?
Target Milestone: --- → mozilla61
Assignee | ||
Comment 16•6 years ago
|
||
Oh, I didn't realize emilio had already r+d this and was waiting for review. Landing
Flags: needinfo?(manishearth)
Comment 17•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 06862c7331a4cd3a304a796196f11ac28683033a -d 079e73b36f8b: rebasing 459003:06862c7331a4 "Bug 1451724: Add reftest ; r=emilio" (tip) merging testing/web-platform/meta/MANIFEST.json warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8965710 [details] Bug 1451724: Add reftest ; https://reviewboard.mozilla.org/r/234238/#review243234 I had more certainly not reviewed the test, fwiw. But also not received a review request either, looks like mozreview got confused because it used two be two different patches. r=me for the test with the nits, and ensuring the WPT lint passes (should run on try). ::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y-ref.html:2 (Diff revision 2) > +<!doctype html> > +<meta charset=utf-8> Needs a title as well. ::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:3 (Diff revision 2) > +<!doctype html> > +<meta charset=utf-8> > +<title></title> Needs a title, author, and spec link. ::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:12 (Diff revision 2) > + background:red; > + width: 200px; > + height: 200px; > +} > +</style> > +<body> no need for body tags. ::: testing/web-platform/tests/css/css-transforms/css-transform-animate-translate-implied-y.html:14 (Diff revision 2) > + height: 200px; > +} > +</style> > +<body> > +<div id=box></div> > +<script type="text/javascript"> No need for type=
Reporter | ||
Comment 19•6 years ago
|
||
So i just tested in 60.0b13 (beta channel) and the issue appears to have been fixed. :) I'm a little confused by: Regression bug RESOLVED as FIXED for Firefox 61 found in Firefox 60 Could someone clarify whether or not this fix will land in the Firefox 60 ESR/ Firefox 60.1 ESR? First time filing as bug for FF and struggling to understand how the ESR fits into the release cycle, thanks
Comment 20•6 years ago
|
||
RESOLVED FIXED refers to when a patch lands on our development branch (mozilla-central), which is currently tracking Firefox 61. I can confirm that the testcase from the initial report indeed works fine on a current Nightly build. It's a bit of a surprise if 60.0b13 is working correctly now for you since the patch from this bug hasn't been backported to Fx60 yet. FWIW, I can still reproduce the bug when testing locally with 60.0b13 (and as I mentioned before, I was able to verify that Nightly builds are fixed). To answer your question RE: ESR60, anything that's fixed in Fx60 will be fixed in ESR60 also. They won't diverge until very close to release. So if this patch gets backported to 60, it'll be in the ESR as well.
Reporter | ||
Comment 21•6 years ago
|
||
My Bad, bug is still there, i forgot that i put a work around in our code. D'oh
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•6 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a993e6e8d14c Add reftest ; r=emilio
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a993e6e8d14c
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10530 for changes under testing/web-platform/tests
Upstream PR merged
Comment 30•6 years ago
|
||
Probably too late to backport to Fx60 at this point given that the first RC build was just created today...
status-firefox-esr60:
--- → affected
Flags: in-testsuite? → in-testsuite+
Updated•6 years ago
|
Comment 31•6 years ago
|
||
Do we want this on esr60 or should we wontfix for that branch?
Flags: needinfo?(manishearth)
Assignee | ||
Comment 32•6 years ago
|
||
I think we should backport but not sure
Flags: needinfo?(manishearth) → needinfo?(emilio)
Comment 33•6 years ago
|
||
Comment on attachment 8965710 [details] Bug 1451724: Add reftest ; I think if it can be in 60 it'd be nice. Note that the approval request is for https://hg.mozilla.org/mozilla-central/rev/03d22e687292, but mozreview got confused and forgot about that revision afterwards. :( [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: trivial transform animation fix. User impact if declined: Potential glitches. Fix Landed on Version: 61 Risk to taking this patch (and alternatives if risky): not really risky. String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(emilio)
Updated•6 years ago
|
tracking-firefox-esr60:
--- → 61+
Comment 34•6 years ago
|
||
Comment on attachment 8965710 [details] Bug 1451724: Add reftest ; stylo fix, approved for 60.1esr
Attachment #8965710 -
Flags: approval-mozilla-esr60+
Comment 35•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/43df2f692b63 https://hg.mozilla.org/releases/mozilla-esr60/rev/e784c8cff0c3
You need to log in
before you can comment on or make changes to this bug.
Description
•