Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(631)

Issue 157523002: Fix incorrect animation for some values of background-position. (Closed)

Created:
6 years, 10 months ago by andersr
Modified:
6 years, 10 months ago
CC:
blink-reviews, shans, rjwright, Mike Lawther (Google), Timothy Loh, darktears, Steve Block, dino_apple.com, Eric Willigers
Base URL:
https://chromium.googlesource.com/chromium/blink.git@backgroundposition
Visibility:
Public.

Description

Fix incorrect animation for some values of background-position. There is a bug which causes incorrect animation for background-position values with right/bottom edge origin. This is because we interpolate the offset value itself, but do not take into account that the offset value represents different positions based on the edge origin of the offset. Example: If we want to animate from "left 10px" to "right 20px", we interpolate 10px to 20px, but "forget" the edge affinitiy of the second value, and end up with a value offset from the left. Fixed by emitting calculated lengths for bottom/right edge origins during animation, such that "right N" or "bottom N" becomes "calc(100% - N)". BUG=341440 TEST=automated Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166997

Patch Set 1 #

Total comments: 1

Patch Set 2 : Zoom properly, use interpolation test instead of reftest. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -2 lines) Patch
A LayoutTests/animations/interpolation/background-position-origin-interpolation.html View 1 1 chunk +140 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/background-position-origin-interpolation-expected.txt View 1 1 chunk +46 lines, -0 lines 0 comments Download
M Source/core/animation/css/CSSAnimatableValueFactory.cpp View 1 2 chunks +13 lines, -2 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
andersr
6 years, 10 months ago (2014-02-07 13:14:46 UTC) #1
pdr.
Steve, Alan, ping? This has been waiting for a little while.
6 years, 10 months ago (2014-02-10 20:50:04 UTC) #2
dstockwell
On 2014/02/10 20:50:04, pdr wrote: > Steve, Alan, ping? This has been waiting for a ...
6 years, 10 months ago (2014-02-10 22:24:22 UTC) #3
alancutter (OOO until 2018)
Thanks for fixing this! lgtm with nits. Could the test be moved to LayoutTests/animation/interpolation/background-position-origin-interpolation.html? https://codereview.chromium.org/157523002/diff/1/Source/core/animation/css/CSSAnimatableValueFactory.cpp ...
6 years, 10 months ago (2014-02-11 00:23:22 UTC) #4
dstockwell
On 2014/02/11 00:23:22, alancutter wrote: > Could the test be moved to > LayoutTests/animation/interpolation/background-position-origin-interpolation.html? ...
6 years, 10 months ago (2014-02-11 00:26:13 UTC) #5
alancutter (OOO until 2018)
On 2014/02/11 00:26:13, dstockwell wrote: > On 2014/02/11 00:23:22, alancutter wrote: > > Could the ...
6 years, 10 months ago (2014-02-11 01:10:31 UTC) #6
andersr
Thanks for reviewing. > interpolation test Yes, I noticed that this test type existed *after* ...
6 years, 10 months ago (2014-02-11 08:32:32 UTC) #7
andersr
PTAL, gentlemen.
6 years, 10 months ago (2014-02-11 09:34:07 UTC) #8
alancutter (OOO until 2018)
On 2014/02/11 09:34:07, andersr wrote: > PTAL, gentlemen. lgtm.
6 years, 10 months ago (2014-02-12 00:21:13 UTC) #9
andersr
The CQ bit was checked by andersr@opera.com
6 years, 10 months ago (2014-02-12 08:59:44 UTC) #10
andersr
The CQ bit was unchecked by andersr@opera.com
6 years, 10 months ago (2014-02-12 08:59:45 UTC) #11
andersr
The CQ bit was checked by andersr@opera.com
6 years, 10 months ago (2014-02-12 08:59:48 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andersr@opera.com/157523002/110001
6 years, 10 months ago (2014-02-12 09:00:00 UTC) #13
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-12 09:16:03 UTC) #14
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=15816
6 years, 10 months ago (2014-02-12 09:16:04 UTC) #15
dstockwell
rslgtm
6 years, 10 months ago (2014-02-12 09:30:59 UTC) #16
dstockwell
The CQ bit was checked by dstockwell@chromium.org
6 years, 10 months ago (2014-02-12 09:31:04 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andersr@opera.com/157523002/110001
6 years, 10 months ago (2014-02-12 09:31:18 UTC) #18
commit-bot: I haz the power
6 years, 10 months ago (2014-02-12 10:56:33 UTC) #19
Message was sent while issue was closed.
Change committed as 166997

Powered by Google App Engine
This is Rietveld 408576698