Closed
Bug 1239564
Opened 8 years ago
Closed 8 years ago
CSS Animation of transform: scale on element with border-radius broken with HWA disabled
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: reesewill, Assigned: chenpighead)
References
Details
(Keywords: regression, testcase)
Attachments
(8 files, 2 obsolete files)
1.68 MB,
video/mp4
|
Details | |
51.08 KB,
video/mp4
|
Details | |
55.24 KB,
video/mp4
|
Details | |
584 bytes,
text/html
|
Details | |
434 bytes,
text/html
|
Details | |
1.23 KB,
patch
|
chenpighead
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
roc
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
698 bytes,
patch
|
mattwoodrow
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36 Steps to reproduce: Bug can be reproduced by using a css animation to resize an element with transform scale. The element must have a border-radius property set. I have only seen this occur on Windows 7, Firefox 43. https://jsfiddle.net/4wdoxfaf/ Actual results: Instead of the element growing or shrinking, it translates diagonally, as though the transform origin is shifting. Expected results: The element should have stayed in its position, the actual size shrinking or growing.
Component: Untriaged → Layout
Priority: P3 → --
Product: Firefox → Core
Comment 1•8 years ago
|
||
(In reply to reesewill from comment #0) > Bug can be reproduced by using a css animation to resize an element with > transform scale. The element must have a border-radius property set. I have > only seen this occur on Windows 7, Firefox 43. > > https://jsfiddle.net/4wdoxfaf/ I couldn't reproduce the described problem. This testcase doesn't have border radius.
Flags: needinfo?(reesewill)
I've updated the test case. Apologies about the first one: https://jsfiddle.net/4wdoxfaf/1/ One more detail... at least one of the 2 machines I've seen this on was 32-bit. I can't be sure about the other one. This is where I originally noticed the bug. You'll notice the problem on the third and sixth example spinner: http://cssload.net/en/spinners
Flags: needinfo?(reesewill)
Comment 3•8 years ago
|
||
I couldn't reproduce. I tried on Mac and Windows. Nightly and release. The Windows machine was using 32bit Firefox, but it's running 64bit Windows.
I'll make a video of the big next Monday. After I first saw the bug on my coworkers Windows notebook. I recreated it on microsofts Windows 7 i.e. 9 official virtual machine for Mac Vm virtual box. https://dev.windows.com/en-us/microsoft-edge/tools/vms/mac/ I'll also get the complete specs of my coworkers notebook on Monday.
Comment 10•8 years ago
|
||
It's a regression: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e747377d86eb4297f5be44aab1d911978dce1c5c&tochange=f590a95a2c64dd420d25d07f081870f2cec26b4d My guess goes to bug 1149923.
Blocks: 1149923
Status: UNCONFIRMED → NEW
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox46:
--- → affected
tracking-firefox45:
--- → ?
tracking-firefox46:
--- → ?
Ever confirmed: true
Flags: needinfo?(jeremychen)
Keywords: regression
Summary: Windows Firefox 43: CSS Animation of transform: scale on element with border-radius broken → CSS Animation of transform: scale on element with border-radius broken with HWA disabled
Assignee | ||
Comment 12•8 years ago
|
||
It can be reproduced on my Ubuntu machine (ff43-release no matter HWA is disabled or not). But it can't be reproduced on my mac (ff43-release)...hmmm. I'll keep digging and find out if bug 1149923 is the root cause or not.
Flags: needinfo?(jeremychen)
Comment 13•8 years ago
|
||
Yes, it was just a guess, maybe a different bug in the changelog is the underlying regression.
Assignee | ||
Comment 14•8 years ago
|
||
Quick update. In bug 1149923, only CompositorOGL::DrawQuad [1] is considered, BasicCompositor::DrawQuad [2]is not. When HWA is enabled, image w/ mask layer would be rendered by [1]; however, when HWA is disabled, image w/ mask layer would be rendered by [2]. I'm working on a patch to take HWA disabled into consideration as well. Sorry about the regression. [1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?from=compositorogl.cpp#1065 [2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#401
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•8 years ago
|
||
After digging deeper, I think my patch in bug 1149923 is not the root cause of this regression. This bug seems more likely a former regression which is revealed after my patch landed. I'll upload a patch and ask for feedback to prove my assumption.
Assignee | ||
Comment 16•8 years ago
|
||
In [1], newTransform calls postTranslate() to translate to RenderTarget's origin; however, maskTransform calls preTranslate() in [2]. I think this is the root cause of this bug. Hi Matt, I'm wondering if maskTransform should call postTranslate() to agree with that of newTransform? [1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#391 [2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#402
Attachment #8713541 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2dd67f6426
Comment 18•8 years ago
|
||
Comment on attachment 8713541 [details] [diff] [review] Part1: Post translate maskSurface to renderTarget coord. (v1) Review of attachment 8713541 [details] [diff] [review]: ----------------------------------------------------------------- Good catch!
Attachment #8713541 -
Flags: feedback?(matt.woodrow) → feedback+
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8713541 [details] [diff] [review] Part1: Post translate maskSurface to renderTarget coord. (v1) Matt, thank you for the feedback. :)
Attachment #8713541 -
Flags: review?(roc)
Comment on attachment 8713541 [details] [diff] [review] Part1: Post translate maskSurface to renderTarget coord. (v1) Review of attachment 8713541 [details] [diff] [review]: ----------------------------------------------------------------- Needs a test!
Attachment #8713541 -
Flags: review?(roc) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Testing steps should be as follows: 1. turn HWA pref off 2. launch browser w/ HWA off 3. open test page and reference page to validate the result I'm still figuring out how to write this test. Note that HWA preference is a "read once" preference, so We'll need to push preference "before" launching browser. However, it seems that reftest and mochitest could not do this. =(
Comment 22•8 years ago
|
||
Jeremy, do you have an eta when this patch will land? thanks
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #22) > Jeremy, do you have an eta when this patch will land? thanks Here's the progress updated on 2/7. I can do re-launching browser w/ specific prefs in marionette test, but the screenshot api seems not work as I expected. I'll continue to finish the test on 2/17, after I return from Chinese New Year. I think I can make it by 2/20.
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 25•8 years ago
|
||
> Here's the progress updated on 2/7. I can do re-launching browser w/
> specific prefs in marionette test, but the screenshot api seems not work as
> I expected.
>
> I'll continue to finish the test on 2/17, after I return from Chinese New
> Year. I think I can make it by 2/20.
Looks like every result of first paint would be correct, and then the composed reseult got messed very soon (if the patch of this bug is not applied). The screenshot taken from marionette test seems always present that correct one, so test can't present what I expected, a broken one. Keep digging in the rendering pipeline...
Assignee | ||
Comment 26•8 years ago
|
||
Ok, it seems more clear to me after tracing the code path back and forth. As Comment 21 said, it take two requirements to make the test: 1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed) 2. Make a screenshot to get a reference image for comparison If we could fulfill both, then test could be made. (verify whether HWA-ON result is consistent with HWA-Off could be one choice) However, I got few problems in the 2nd requirement. Problem 1: Can't trigger MakeSnapShot() to get compositor result. When calling marionette.screenshot() with chrome context in [1], some flags may cause MakeSnapShotIfRequired() in [2] early returned, so we can't make a snapshot from parent side. In this case, the returned screenshot may be composed in client side alone (by moz2D api I guess), which can't present the real composed result on the screen since the code path would never go through compositor parent (neither BasicCompositor nor CompositorOGL). After I did some hack to force marionette.screenshot() successfully go through compositor parent, I jumped into another problem. Problem 2: Can't make a screenshot for failed test case. After successfully calling MakeSnapShot(), in compositor parent side, we always get a new clean mRenderTarget with mOrigin (0,0), which leads to a offset=(0,0) in [3]. However, this bug can only reproduced w/ non-zero offset (see [4]). This could be proved by switching to test page from any other tab, you'll see a correct rendered result at a glance. In conclusion, even we force marionette.screenshot() to go MaksSnapShot() code path and get the compositor result, the screenshot would never show a failed result. Maybe I can try Bug 1173117 to use OS level snapshot, but it seems only work in windows? Or, I'm not sure if we should make this test as a followup bug? Hi roc, is there anything else I could try? [1] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/testing/marionette/driver.js#2586 [2] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/client/ClientLayerManager.cpp#494 [3] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/basic/BasicCompositor.cpp#366 [4] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/basic/BasicCompositor.cpp#411
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #26) > 1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed) You don't need to force this. You can assume we'll run some tests with HWA off. As long as your test passes with HWA on and off, we'll be fine. We should be able to create a reftest for this similar to other animation reftests.
Flags: needinfo?(roc)
Assignee | ||
Comment 28•8 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #26) > > 1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed) > > You don't need to force this. You can assume we'll run some tests with HWA > off. As long as your test passes with HWA on and off, we'll be fine. > > We should be able to create a reftest for this similar to other animation > reftests. You're right, I should rephrase what I said in Comment 26. The first requirement may be optional if we already have tests for both HWA on and off. However, I'm wondering if we could not fulfill the second requirement, how can we make a snapshot for broken compositor result? Here is the exact what happens in BasicCompositor while reproducing this bug: 1. Start first time composition in BasicCompositor with a clean mRenderTarget. Since this is the first time composition, mRenderTarget should always be the whole viewport with mOrigin = (0,0). 2. Refresh driver triggers next tick due to whatever reason, this time only partial region, where the mask layer positioned, would be updated. 3. Start second time composition, this time only update where mask layer is positioned. So, the same mRenderTarget with non-zero mOrigin is used for BasicCompositor to do partial updating. Then, the non-zero mOrigin leads to a non-zero offset translate in mask layer painting, and then causes the broken result. This means we can't get the first time composition go through the broken code path. In Comment 26, I stated that the testing screenshot API is either doing composition in content side by Moz2D, or doing so in parent side by sending MakeSnapShot() to compositor. Doing composition in content side of course will not go through the broken code path since it never go through parent's compositor. Sending MakeSnapShot() to compositor means asking compositor to do a one time composition, which will always like the first time composition (like the step 1 mentioned above). Neither ways can go through the broken code path. =(
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #28) > However, I'm wondering if we could not fulfill the second requirement, how > can we make a snapshot for broken compositor result? > > Here is the exact what happens in BasicCompositor while reproducing this bug: > 1. Start first time composition in BasicCompositor with a clean > mRenderTarget. Since this is the first time composition, mRenderTarget > should always be the whole viewport with mOrigin = (0,0). > 2. Refresh driver triggers next tick due to whatever reason, this time only > partial region, where the mask layer positioned, would be updated. > 3. Start second time composition, this time only update where mask layer is > positioned. So, the same mRenderTarget with non-zero mOrigin is used for > BasicCompositor to do partial updating. Then, the non-zero mOrigin leads to > a non-zero offset translate in mask layer painting, and then causes the > broken result. > > This means we can't get the first time composition go through the broken > code path. Reftest snapshots use the compositor, and they support partial updates. So you could write a reftest that uses reftest-wait and then triggers an incremental update in the MozReftestInvalidate event. That should work.
Flags: needinfo?(roc)
Assignee | ||
Comment 30•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7b48fb8ed3
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dea4160f0e0
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8713541 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8721955 -
Attachment is obsolete: true
Assignee | ||
Comment 34•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8721957 -
Attachment description: Part2: Add reftest for mask layer composition. (v2) → Part2: Add reftest for mask layer composition. (v1)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8721957 [details] [diff] [review] Part2: Add reftest for mask layer composition. (v1) Since we already have reftest-no-accel on try, this test should work. Use fuzzy due to few distorted pixels caused by border-radius. roc, thank you for pointing me to the right direction.
Attachment #8721957 -
Flags: review?(roc)
Attachment #8721957 -
Flags: review?(roc) → review+
Comment hidden (obsolete) |
Assignee | ||
Comment 39•8 years ago
|
||
try looks positove: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de14450423b except following known issues: Linux x64 debug tc-M-e10s[Tier-2] 10, bc4 => Bug 1227730 Windows 7 debug R-e10s => Bug 1145903 Both are not related to this bug. So I'm setting the checkin-needed flag.
Keywords: checkin-needed
Comment 40•8 years ago
|
||
failed to apply: renamed 1239564 -> Bug-1239564---Post-translate-maskSurface-to-fender.patch applying Bug-1239564---Post-translate-maskSurface-to-fender.patch patching file gfx/layers/basic/BasicCompositor.cpp Hunk #1 FAILED at 428 1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/basic/BasicCompositor.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug-1239564---Post-translate-maskSurface-to-fender.patch
Flags: needinfo?(jeremychen)
Keywords: checkin-needed
Comment 42•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd1ed930b50 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8df59280f9
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7dd1ed930b50 https://hg.mozilla.org/mozilla-central/rev/2c8df59280f9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 44•8 years ago
|
||
Too late for 45, Jeremy, do you want to uplift this to 46?
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 45•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #44) > Too late for 45, Jeremy, do you want to uplift this to 46? Sounds good to me. I'm setting the approval request.
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8721956 [details] [diff] [review] Part1: Post translate maskSurface to renderTarget. (v2, carry r+:roc) Approval Request Comment [Feature/regressing bug #]:layout rendering w/o hardware acceleration [User impact if declined]: Users may see an ill-composed result on screen when setting css attributes like transform and/or animation. [Describe test coverage new/current, TreeHerder]: [Risks and why]:low risk [String/UUID change made/needed]:none
Attachment #8721956 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8721957 [details] [diff] [review] Part2: Add reftest for mask layer composition. (v1) Approval Request Comment [Feature/regressing bug #]:layout reftest [User impact if declined]: Ill-composed result may show on screen w/o HWA if we can't test them in advance. [Describe test coverage new/current, TreeHerder]: [Risks and why]:low risk [String/UUID change made/needed]:none
Attachment #8721957 -
Flags: approval-mozilla-beta?
Comment 48•8 years ago
|
||
Comment on attachment 8721956 [details] [diff] [review] Part1: Post translate maskSurface to renderTarget. (v2, carry r+:roc) Fix for users with out HWA. Includes new reftest, ok to uplift to beta.
Attachment #8721956 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 49•8 years ago
|
||
Comment on attachment 8721957 [details] [diff] [review] Part2: Add reftest for mask layer composition. (v1) More tests sounds good to me.
Attachment #8721957 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 50•8 years ago
|
||
You don't need to set checkin-needed for uplifts :)
Keywords: checkin-needed
Comment 51•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8388d17e4503 https://hg.mozilla.org/releases/mozilla-beta/rev/f2d4c73f4e96
Assignee | ||
Comment 52•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #50) > You don't need to set checkin-needed for uplifts :) Just got Daily Release Tracking Alert email, and thought maybe there's something that I should have done. Thank you for telling me. :)
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Comment 53•8 years ago
|
||
[bugday-20160323] Able to get expected results for: Status: RESOLVED,FIXED -> VERIFIED Component: Name Firefox Version 46.0b2 Build ID 20160314144540 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Actual Results: Element at its place while growing and shrinking. border: 2px, --working border-radius: 234px, --working Expected Results: CSS animation is working and responding to border-radius in HWA disabled mode.
Comment 54•8 years ago
|
||
Is there a reason why, in the reftest added in part 2, the ref does not use translateZ at all, but the test does? This is seemingly causing it to fail under some content backends (i.e. Skia), whereas either removing the translateZ or ensuring it is present in both allows it to pass.
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 55•8 years ago
|
||
(In reply to Lee Salzman [:lsalzman] from comment #54) > Is there a reason why, in the reftest added in part 2, the ref does not use > translateZ at all, but the test does? This is seemingly causing it to fail > under some content backends (i.e. Skia), whereas either removing the > translateZ or ensuring it is present in both allows it to pass. We can't remove translateZ since using translateZ plus will-change could ensure masklayers not been merged in content side and force the code flow go through compositor side. After I did some testing, ensuring translateZ been present in both test and ref should cover this bug as well. I'll upload a patch later.
Flags: needinfo?(jeremychen)
Assignee | ||
Comment 56•8 years ago
|
||
Let transform property in reference file consistent with that in test file. Hi Matt, I make this change due to Comment 54. Could you help review this? Thank you.
Attachment #8734217 -
Flags: review?(matt.woodrow)
Updated•8 years ago
|
Attachment #8734217 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 58•8 years ago
|
||
Hi lsalzman, could you provide a link for the try-fail you mentioned in Comment 54?
Flags: needinfo?(lsalzman)
Comment 59•8 years ago
|
||
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #58) > Hi lsalzman, could you provide a link for the try-fail you mentioned in > Comment 54? https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fbd63502b0d
Flags: needinfo?(lsalzman)
Comment 60•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3062d172679f
Assignee | ||
Comment 61•8 years ago
|
||
Comment on attachment 8734217 [details] [diff] [review] Bug 1239564 - fix reftest. This patch fix the test-fail under some content backends (i.e. Skia). Please see Comment 54 & Comment 59. Since this test has been landed in beta, set uplift requests for both aurora and beta. Approval Request Comment [Feature/regressing bug #]: layout reftest [User impact if declined]: Ill-composed result may show on screen w/o HWA if we can't test them in advance. [Describe test coverage new/current, TreeHerder]: [Risks and why]: low [String/UUID change made/needed]: none
Attachment #8734217 -
Flags: approval-mozilla-beta?
Attachment #8734217 -
Flags: approval-mozilla-aurora?
Comment 62•8 years ago
|
||
Comment on attachment 8734217 [details] [diff] [review] Bug 1239564 - fix reftest. Test fix, NPOTB, ok to uplift
Attachment #8734217 -
Flags: approval-mozilla-beta?
Attachment #8734217 -
Flags: approval-mozilla-beta+
Attachment #8734217 -
Flags: approval-mozilla-aurora?
Attachment #8734217 -
Flags: approval-mozilla-aurora+
Comment 63•8 years ago
|
||
Tested on Windows 10 x64, on the build from 2016-01-13, it was reproducible. Retested in on 47.0b3 and the bug was fixed. [testday-20160506]
You need to log in
before you can comment on or make changes to this bug.
Description
•