|
|
Created:
7 years, 1 month ago by davve Modified:
7 years, 1 month ago CC:
blink-reviews, rjwright, Mike Lawther (Google), zoltan1, eae+blinkwatch, dglazkov+blink, leviw+renderwatch, dstockwell, Timothy Loh, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, bemjb+rendering_chromium.org, dino_apple.com, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@length-relative-die-step-1-4 Visibility:
Public. |
DescriptionConvert animation and renderer code to know about BorderImageLength
This propagates BorderImageLengthBox further out in the code, but
actually converts it to an actual number or length as soon as possible
in the animation code, and animates it as the individual type.
Now we can also remove the compatibility conversion methods between
BorderImageLengthBox and LengthBox.
Also, update a couple of "Weird number of spaces at line-start"
indentations in CSSPropertyAnimation.cpp to not make the new code
inconsistent with the old wrt. indentation.
BUG=259107
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161715
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebased on top of patch set #2 of https://codereview.chromium.org/55783002/ #
Total comments: 5
Patch Set 3 : Rebased on top of patch set #5 of https://codereview.chromium.org/55783002/ and review issues fixed. #Patch Set 4 : Rebased to latest master #
Messages
Total messages: 18 (0 generated)
Gave a quick look at the change. I think the transition should be covered by tests (assuming that it's not already). https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; I don't think this is totally correct. In this case, we can still interpolate from a Length to a number or vice-versa: Lengths being treated as real numbers for the purpose of interpolation (http://dev.w3.org/csswg/css-transitions/#animatable-types). It's probably gross but Length::blend handles the interpolation correctly so you would just have to consider a number as Fixed Length and that should give you the right result (but it will need to be converted back to a number or else we lose some information). https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R... File Source/core/rendering/style/RenderStyle.h (right): https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R... Source/core/rendering/style/RenderStyle.h:477: LengthOrNumberBox borderImageOutset() const { return surround->border.image().outset(); } const LengthOrNumberBox& to avoid copies (if aware).
> One thing to note is that AnimatableLengthBox will animate boxes of > numbers too, making the box slightly misnamed. In practice, it's > indeed oblivious to the type of the four animation values it contains. > It's not a given that it is worth the trouble to rename it to > AnimatableBox since we have new animation code coming around the > corner. I'm not sure I understand the last sentence. I would encourage renaming the class to AnimatableBox if it is going to end up containing things other than Lengths.
On 2013/11/02 00:36:01, alancutter wrote: > > One thing to note is that AnimatableLengthBox will animate boxes of > > numbers too, making the box slightly misnamed. In practice, it's > > indeed oblivious to the type of the four animation values it contains. > > It's not a given that it is worth the trouble to rename it to > > AnimatableBox since we have new animation code coming around the > > corner. > > I'm not sure I understand the last sentence. I would encourage renaming the > class to AnimatableBox if it is going to end up containing things other than > Lengths. It already does, but disguised as "relative" lengths. I'm more than willing to do the rename but would it be acceptable to do it in a separate CL?
On 2013/11/02 13:32:25, David Vest wrote: > It already does, but disguised as "relative" lengths. I'm more than willing to > do the rename but would it be acceptable to do it in a separate CL? Filed 314470 for this.
https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > I don't think this is totally correct. In this case, we can still interpolate > from a Length to a number or vice-versa: Lengths being treated as real numbers > for the purpose of interpolation > (http://dev.w3.org/csswg/css-transitions/#animatable-types). > > It's probably gross but Length::blend handles the interpolation correctly so you > would just have to consider a number as Fixed Length and that should give you > the right result (but it will need to be converted back to a number or else we > lose some information). If we're interpolating from a length to a number, then we're doing something like: line-height: 50px; to line-height: 4; (which means 4x font-size) We could convert the 4 to a pixel value (say 40px if font-size is 10px), then interpolate, and this would work; but how do we represent the result in the computed style? Furthermore, how do we inherit the result correctly? In particular, if we're at 50% then this should be a value which is half-way between 50px and four times whatever the current font-size is (i.e. a value that is responsive to changes in font-size). We could use a calc expression in the computed style for this (calc(25px + 200%)), but this is an unreachable value (there are no specified styles that one could use to generate this computed style), and will have a different effect if used as a specified style. I think the current behaviour is probably the best we can do right now.
On 2013/11/03 23:59:42, shans wrote: > https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... > File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): > > https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... > Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; > On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > > I don't think this is totally correct. In this case, we can still interpolate > > from a Length to a number or vice-versa: Lengths being treated as real numbers > > for the purpose of interpolation > > (http://dev.w3.org/csswg/css-transitions/#animatable-types). > > > > It's probably gross but Length::blend handles the interpolation correctly so > you > > would just have to consider a number as Fixed Length and that should give you > > the right result (but it will need to be converted back to a number or else we > > lose some information). > > If we're interpolating from a length to a number, then we're doing something > like: > line-height: 50px; > to > line-height: 4; > (which means 4x font-size) > > We could convert the 4 to a pixel value (say 40px if font-size is 10px), then > interpolate, and this would work; but how do we represent the result in the > computed style? Furthermore, how do we inherit the result correctly? > > In particular, if we're at 50% then this should be a value which is half-way > between 50px and four times whatever the current font-size is (i.e. a value that > is responsive to changes in font-size). We could use a calc expression in the > computed style for this (calc(25px + 200%)), but this is an unreachable value > (there are no specified styles that one could use to generate this computed > style), and will have a different effect if used as a specified style. > > I think the current behaviour is probably the best we can do right now. Tim Loh just pointed out that line-height doesn't hit this code path. Let's try again... If we're interpolating from a length to a number, then we're doing something like: border-image-width: 50px; to border-image-width: 4; (which means 4x border-width) We could convert the 4 to a pixel value (say 40px if font-size is 10px), then interpolate, and this would work; but how do we represent the result in the computed style? Furthermore, how do we inherit the result correctly? In particular, if we're at 50% then this should be a value which is half-way between 50px and four times whatever the current border-width is (i.e. a value that is responsive to changes in border-width). We can't use a calc expression in the computed style for this (calc(25px + 200%)) because this means 25px + twice the width of the box.
https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > I don't think this is totally correct. In this case, we can still interpolate > from a Length to a number or vice-versa: Lengths being treated as real numbers > for the purpose of interpolation > (http://dev.w3.org/csswg/css-transitions/#animatable-types). But no worse than the current code though? (see LengthBox variant of blendFunc) for the current type check. If not worse than current, can we handle this in a separate bug/issue? https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R... File Source/core/rendering/style/RenderStyle.h (right): https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R... Source/core/rendering/style/RenderStyle.h:477: LengthOrNumberBox borderImageOutset() const { return surround->border.image().outset(); } On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > const LengthOrNumberBox& to avoid copies (if aware). OK.
On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > Gave a quick look at the change. I think the transition should be covered by > tests (assuming that it's not already). The animation and interpolation is covered by LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}* tests. At least the length case is covered. The number case seems to be currently non-testable using that test framework because of bug http://crbug.com/304000.
lgtm https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; On 2013/11/04 15:02:28, David Vest wrote: > On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote: > > I don't think this is totally correct. In this case, we can still interpolate > > from a Length to a number or vice-versa: Lengths being treated as real numbers > > for the purpose of interpolation > > (http://dev.w3.org/csswg/css-transitions/#animatable-types). > > But no worse than the current code though? (see LengthBox variant of blendFunc) > for the current type check. If not worse than current, can we handle this in a > separate bug/issue? Fine point. Agreed to punt this to a separate issue, especially considering that the testing is blocked on a bug! @shans, I think your proposal would work... that is assuming you don't also animate 'border-width'. https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; This is missing a FIXME per our discussion. https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm... File Source/platform/BorderImageLengthBox.h (right): https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm... Source/platform/BorderImageLengthBox.h:44: BorderImageLengthBox(Length t, Length r, Length b, Length l) Do we still want to keep this constructor? https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm... Source/platform/BorderImageLengthBox.h:60: BorderImageLengthBox(const BorderImageLength& t, const BorderImageLength& r, const BorderImageLength& b, const BorderImageLength& l) Remember my stick about |o|, well it applies also to |t|, |r|, |b| and |l| (and most one letter variable names)
> The animation and interpolation is covered by > LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}* > tests. At least the length case is covered. The number case seems to be > currently non-testable using that test framework because of bug > http://crbug.com/304000. What exactly is the problem? Can you file a bug? I have plans to beef up those tests so will look into it.
On 2013/11/05 03:16:12, Steve Block wrote: > > The animation and interpolation is covered by > > LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}* > > tests. At least the length case is covered. The number case seems to be > > currently non-testable using that test framework because of bug > > http://crbug.com/304000. > What exactly is the problem? Can you file a bug? I have plans to beef up those > tests so will look into it. Not sure there is a bug to file. It looked like the framework tried setting the expected value in some manner, but because of http://crbug.com/304000 no non-integer values can be set (since the css parser doesn't allow it yet). When fixing that bug, there should be no issue.
https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat... File Source/core/frame/animation/CSSPropertyAnimation.cpp (right): https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat... Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to; On 2013/11/04 18:15:54, Julien Chaffraix - PST wrote: > This is missing a FIXME per our discussion. OK. https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm... File Source/platform/BorderImageLengthBox.h (right): https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm... Source/platform/BorderImageLengthBox.h:60: BorderImageLengthBox(const BorderImageLength& t, const BorderImageLength& r, const BorderImageLength& b, const BorderImageLength& l) On 2013/11/04 18:15:54, Julien Chaffraix - PST wrote: > Remember my stick about |o|, well it applies also to |t|, |r|, |b| and |l| (and > most one letter variable names) OK.
OK, got it. For the short term, I'll add some testing that uses integer numbers.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/55813002/290001
Message was sent while issue was closed.
Change committed as 161715
Message was sent while issue was closed.
I added some additional changes in https://codereview.chromium.org/54123007 It looks like http://crbug.com/304000 can be closed now?
Message was sent while issue was closed.
> I added some additional changes in https://codereview.chromium.org/54123007 s/changes/tests |