Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
983073002:
Make font-size-adjust animatable. (Closed)
|
Created:
5 years, 9 months ago by changseok Modified:
5 years, 7 months ago CC:
dstockwell, darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, dglazkov+blink, ed+blinkwatch_opera.com, Mike Lawther (Google), rjwright, rwlbuis, shans, Steve Block Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMake font-size-adjust animatable.
This change brings animation support to the font-size-adjust css property.
BUG=451346
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192116
Patch Set 1 #
Total comments: 9
Patch Set 2 : Removed unnecessary code and comments. #Patch Set 3 : Updated the comment in FontBuilder.cpp #Patch Set 4 : Do not allow font-size-adjust to animate for none or 0 #
Total comments: 13
Patch Set 5 : Just rebased. #Patch Set 6 : Addressed comments #
Total comments: 2
Patch Set 7 : Rebased #Patch Set 8 : Changed to use fallBackToLegacy in StringKeyframe.cpp #
Total comments: 2
Patch Set 9 : Rebased. #Patch Set 10 : Rebase expectations for bug2886-2.html #Messages
Total messages: 40 (10 generated)
shivamidow@gmail.com changed reviewers: + eae@chromium.org, rune@opera.com, timloh@chromium.org
Please have a look.
https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return createFromDouble(style.fontSizeAdjust()); Shouldn't you use the font's aspect value for 0 to get a smooth transition from 'none' to to <number>? https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... File Source/core/css/resolver/FontBuilder.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:338: // To get an accurate x-height from a font cache while animation, we need to reset adjustedSize ... while animating ... https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:340: // and which is affected by effectiveFontSize. -and https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:341: fontDescription.setAdjustedSize(0); This is necessary in general to get the correct result when calling updateFont(), hence createFont(), multiple times on the same FontBuilder. This happens to be the case for animations only at the moment, but the comment should probably say something along the lines of: "The aspect value is based on the x-height of the font for the computed font size, so we need to reset the adjustment." Would that be correct?
In the CL description skip "an" in "brings an animation support".
https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return createFromDouble(style.fontSizeAdjust()); On 2015/03/06 10:08:05, rune wrote: > Shouldn't you use the font's aspect value for 0 to get a smooth transition from > 'none' to to <number>? It takes some time to understand this comment. Current implementation is somewhat different from Gecko's behavior. Gecko doesn't let font-size-adjust animate for 0 or none as you pointed out. But I think this is something that needs more general approach for keyframe animation. For 'perspective' css property, it also shows us different behavior from Gecko's where any keyframe has 'none' or unexpected value for the property value. @-webkit-keyframes fontPerspective { from { -webkit-perspective: none; } to { -webkit-perspective: 100px; } } This keyframe(or course, unprefixed one used for gecko) doesn't work for gecko, but blink does. In short, the animation for unexpected value happens for not only font-size-adjust, but other properties. How about dealing with the issue in a seperated ticket? https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... File Source/core/css/resolver/FontBuilder.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:338: // To get an accurate x-height from a font cache while animation, we need to reset adjustedSize On 2015/03/06 10:08:06, rune wrote: > ... while animating ... Done. https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:340: // and which is affected by effectiveFontSize. On 2015/03/06 10:08:05, rune wrote: > -and Done. https://codereview.chromium.org/983073002/diff/1/Source/core/css/resolver/Fon... Source/core/css/resolver/FontBuilder.cpp:341: fontDescription.setAdjustedSize(0); On 2015/03/06 10:08:06, rune wrote: > This is necessary in general to get the correct result when calling > updateFont(), hence createFont(), multiple times on the same FontBuilder. This > happens to be the case for animations only at the moment, but the comment should > probably say something along the lines of: > > "The aspect value is based on the x-height of the font for the computed font > size, so we need to reset the adjustment." > > Would that be correct? Yes correct. thanks for the summary =)
ericwilligers@chromium.org changed reviewers: + ericwilligers@chromium.org
Please consider adding a case in StringKeyframe.cpp so font-size-adjust can be animated using interpolable values. https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return createFromDouble(style.fontSizeAdjust()); On 2015/03/08 06:16:56, changseok wrote: > On 2015/03/06 10:08:05, rune wrote: > > Shouldn't you use the font's aspect value for 0 to get a smooth transition > from > > 'none' to to <number>? > > It takes some time to understand this comment. Current implementation is > somewhat different from Gecko's behavior. > Gecko doesn't let font-size-adjust animate for 0 or none as you pointed out. But > I think this is something that needs more general approach for keyframe > animation. > For 'perspective' css property, it also shows us different behavior from Gecko's > where any keyframe has 'none' or unexpected value for the property value. > @-webkit-keyframes fontPerspective { > > from { -webkit-perspective: none; } > to { -webkit-perspective: 100px; } > } > This keyframe(or course, unprefixed one used for gecko) doesn't work for gecko, > but blink does. > > In short, the animation for unexpected value happens for not only > font-size-adjust, but other properties. How about dealing with the issue in a > seperated ticket? As it happens, I have a change involving perspective up for review. https://codereview.chromium.org/955863002 (We are deprecating the AnimatableValue classes and implementing InterpolableValues. Unfortunately, we still need AnimatableValue classes for now, e.g. they are currently still used by CSS transitions.) I had noted the unfortunate behavior regarding 'none' in my change description, and included a layout test FIXME: // FIXME: flip between length and none (as Firefox does). font-size-adjust should also flip - at minimum this should be noted with a FIXME.
On 2015/03/08 07:10:15, Eric Willigers wrote: > Please consider adding a case in StringKeyframe.cpp so font-size-adjust can be > animated using interpolable values. > > https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... > File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): > > https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CS... > Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return > createFromDouble(style.fontSizeAdjust()); > On 2015/03/08 06:16:56, changseok wrote: > > On 2015/03/06 10:08:05, rune wrote: > > > Shouldn't you use the font's aspect value for 0 to get a smooth transition > > from > > > 'none' to to <number>? > > > > It takes some time to understand this comment. Current implementation is > > somewhat different from Gecko's behavior. > > Gecko doesn't let font-size-adjust animate for 0 or none as you pointed out. > But > > I think this is something that needs more general approach for keyframe > > animation. > > For 'perspective' css property, it also shows us different behavior from > Gecko's > > where any keyframe has 'none' or unexpected value for the property value. > > @-webkit-keyframes fontPerspective { > > > > > from { -webkit-perspective: none; } > > to { -webkit-perspective: 100px; } > > } > > This keyframe(or course, unprefixed one used for gecko) doesn't work for > gecko, > > but blink does. > > > > In short, the animation for unexpected value happens for not only > > font-size-adjust, but other properties. How about dealing with the issue in a > > seperated ticket? > > As it happens, I have a change involving perspective up for review. > https://codereview.chromium.org/955863002 > > (We are deprecating the AnimatableValue classes and implementing > InterpolableValues. Unfortunately, we still need AnimatableValue classes for > now, e.g. they are currently still used by CSS transitions.) > > > I had noted the unfortunate behavior regarding 'none' in my change description, > and included a layout test FIXME: > // FIXME: flip between length and none (as Firefox does). > > > font-size-adjust should also flip - at minimum this should be noted with a > FIXME. Great. Thanks for the info. I saw the StringKeyframe class and tried it for a while. But I could not test it with my test case. (even I could not see its initialization :P) Would you let me know a sample with which I can test and run to understand how it works? =)
This (and the patch adding font-size-adjust support) seem a bit weird with respect to 'none'. From my reading, 'none' means that there is no adjustment to font-size, while '0' would make every font adjusted to a font-size of 0px. Why are we freely switching between these? 'none' shouldn't be able to be animated as 0.
On 2015/03/08 23:44:48, Timothy Loh wrote: > This (and the patch adding font-size-adjust support) seem a bit weird with > respect to 'none'. From my reading, 'none' means that there is no adjustment to > font-size, while '0' would make every font adjusted to a font-size of 0px. Why > are we freely switching between these? 'none' shouldn't be able to be animated > as 0. I agree. I reluctantly OK'ed 0 as 'none' in the initial review because that's what Gecko does, but really, it should give an adjusted font-size of 0px, as you say. As I commented above, I think 'none' should translate into the font's aspect value when animating between 'none' and a number.
On 2015/03/09 00:02:07, rune (OOO) wrote: > On 2015/03/08 23:44:48, Timothy Loh wrote: > > This (and the patch adding font-size-adjust support) seem a bit weird with > > respect to 'none'. From my reading, 'none' means that there is no adjustment > to > > font-size, while '0' would make every font adjusted to a font-size of 0px. Why > > are we freely switching between these? 'none' shouldn't be able to be animated > > as 0. > > I agree. I reluctantly OK'ed 0 as 'none' in the initial review because that's > what Gecko does, but really, it should give an adjusted font-size of 0px, as you > say. > > As I commented above, I think 'none' should translate into the font's aspect > value when animating between 'none' and a number. You can't really make it animate smoothly unless there's only one font used. The spec says "Animatable: as number", so this should just not be animatable in these cases. So I think for the AnimatableValues codepath we should create an AnimatableUnknown(CSSValueNone) for 'none', and an AnimatableDouble otherwise. You'll also want to update CSSPropertyEquality for transitions to work.
> I saw the StringKeyframe class and tried it for a while. But I could not test it > with my test case. (even I could not see its initialization :P) > Would you let me know a sample with which I can test and run to understand how > it works? =) The easy way to test it is through element.animate, for example see http://updates.html5rocks.com/2014/05/Web-Animations---element-animate-is-now... Currently, the second half of each -expected.html file in animations/interpolation shows the results when using the Web Animations API.
On 2015/03/09 00:21:30, Timothy Loh wrote: > You can't really make it animate smoothly unless there's only one font used. The > spec says "Animatable: as number", so this should just not be animatable in > these cases. This sounds more reasonable to me. I fixed for 0 and none value not to be animated by removing the property from keyframes where it has either 0 or none.
Any comments? =)
Eric@, rune@ Do you have any concern? I'd like to make this go further.
On 2015/03/13 16:08:35, changseok wrote: > Eric@, rune@ > Do you have any concern? I'd like to make this go further. I'll defer to the owners of core/animation
shivamidow@gmail.com changed reviewers: + alancutter@chromium.org, dstockwell@chromium.org, shans@chromium.org
On 2015/03/16 12:46:44, rune wrote: > I'll defer to the owners of core/animation rune, thanks for the ack. =) Added more animation owners. +alancutter, +dstockwell, +shans
Seems like there's still confusion between 0 and none. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, Why can't we interpolate smoothly from 0 to 1.2? https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:54: {at: 0.6, is: 'none'}, This is incorrect, the values should be '1.2' at 0.6, 1 and 1.5. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:66: {at: 0, is: '0.200000002980232'}, just '0.2' should work here, the helper performs rounding https://codereview.chromium.org/983073002/diff/60001/Source/core/animation/cs... File Source/core/animation/css/CSSAnimations.cpp (right): https://codereview.chromium.org/983073002/diff/60001/Source/core/animation/cs... Source/core/animation/css/CSSAnimations.cpp:90: static bool isValidValueForAnimation(const CSSPropertyID& property, const AnimatableValue& value) I do not think you need to make these changes to CSSAnimations.cpp and EffectInput.cpp.
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/16 21:49:31, dstockwell wrote: > Why can't we interpolate smoothly from 0 to 1.2? Because '0' for font-size-adjust is same meaning with 'none'. This is following behavior to the gecko. It treats them as a same value and It doesn't animate for 0 nor none if any frame has the values for font-size-adjust. My first patch allowed font-size-adjust for '0' to animate though, I changed it not to animate after https://codereview.chromium.org/983073002/#msg11. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:54: {at: 0.6, is: 'none'}, On 2015/03/16 21:49:31, dstockwell wrote: > This is incorrect, the values should be '1.2' at 0.6, 1 and 1.5. With same reason above. if any single frame has '0' or 'none' for font-size-adjust, it is intended not to allow animation for the property. Gecko works with the same way as if it doesn't know such animating property. Please run this test in gecko. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:66: {at: 0, is: '0.200000002980232'}, On 2015/03/16 21:49:31, dstockwell wrote: > just '0.2' should work here, the helper performs rounding Cool! https://codereview.chromium.org/983073002/diff/60001/Source/core/animation/cs... File Source/core/animation/css/CSSAnimations.cpp (right): https://codereview.chromium.org/983073002/diff/60001/Source/core/animation/cs... Source/core/animation/css/CSSAnimations.cpp:90: static bool isValidValueForAnimation(const CSSPropertyID& property, const AnimatableValue& value) On 2015/03/16 21:49:31, dstockwell wrote: > I do not think you need to make these changes to CSSAnimations.cpp and > EffectInput.cpp. I guess you assume '0' and 'none' should be interpolated anyway? I just followed gecko's behavior. If we want to make a consistency with gecko, we need to get rid of properties having invalid values from keyframes. Is there a better way by chance?
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/17 at 07:12:58, changseok wrote: > On 2015/03/16 21:49:31, dstockwell wrote: > > Why can't we interpolate smoothly from 0 to 1.2? > > Because '0' for font-size-adjust is same meaning with 'none'. This is following behavior to the gecko. It treats them as a same value and It doesn't animate for 0 nor none if any frame has the values for font-size-adjust. My first patch allowed font-size-adjust for '0' to animate though, I changed it not to animate after https://codereview.chromium.org/983073002/#msg11. This sounds like a bug in gecko, we should clarify the behavior with the CSS working group before blindly following -- given gecko is the only other implementation. If we must treat 0 the same as none, you can use AnimatableDouble::InterpolationIsNonContinuousWithZero in AnimatedStyleBuilder and forceDefaultStyleInterpolation in StringKeyframe to prevent interpolating other numbers with 0. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:54: {at: 0.6, is: 'none'}, On 2015/03/17 at 07:12:58, changseok wrote: > On 2015/03/16 21:49:31, dstockwell wrote: > > This is incorrect, the values should be '1.2' at 0.6, 1 and 1.5. > > With same reason above. if any single frame has '0' or 'none' for font-size-adjust, it is intended not to allow animation for the property. Gecko works with the same way as if it doesn't know such animating property. Please run this test in gecko. For values which do not smoothly interpolate, eg. none and '1.2' the result should step at 0.5. This behavior may not yet be implemented in Gecko but is consistent with all other animation in Blink. See the comments on non-interoperable values in http://dev.w3.org/csswg/css-animations-1/#wg-resolutions-pending
Thanks for the review. It was quite helpful. I updated a patch to address comments. Please have a look again. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/17 07:42:10, dstockwell wrote: > On 2015/03/17 at 07:12:58, changseok wrote: > > On 2015/03/16 21:49:31, dstockwell wrote: > > > Why can't we interpolate smoothly from 0 to 1.2? > > > > Because '0' for font-size-adjust is same meaning with 'none'. This is > following behavior to the gecko. It treats them as a same value and It doesn't > animate for 0 nor none if any frame has the values for font-size-adjust. My > first patch allowed font-size-adjust for '0' to animate though, I changed it not > to animate after https://codereview.chromium.org/983073002/#msg11. > > This sounds like a bug in gecko, we should clarify the behavior with the CSS > working group before blindly following -- given gecko is the only other > implementation. O.K > If we must treat 0 the same as none, you can use > AnimatableDouble::InterpolationIsNonContinuousWithZero in AnimatedStyleBuilder > and forceDefaultStyleInterpolation in StringKeyframe to prevent interpolating > other numbers with 0. I don't have a strong opinion whether or not we should treat '0' as none. But if it should be, I'd like to cover it in a seperated bug later. For now, let's treat 0 as 'none' as gecko does. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:54: {at: 0.6, is: 'none'}, On 2015/03/17 07:42:10, dstockwell wrote: > On 2015/03/17 at 07:12:58, changseok wrote: > > On 2015/03/16 21:49:31, dstockwell wrote: > > > This is incorrect, the values should be '1.2' at 0.6, 1 and 1.5. > > > > With same reason above. if any single frame has '0' or 'none' for > font-size-adjust, it is intended not to allow animation for the property. Gecko > works with the same way as if it doesn't know such animating property. Please > run this test in gecko. > > For values which do not smoothly interpolate, eg. none and '1.2' the result > should step at 0.5. This behavior may not yet be implemented in Gecko but is > consistent with all other animation in Blink. See the comments on > non-interoperable values in > http://dev.w3.org/csswg/css-animations-1/#wg-resolutions-pending I see. Fixed as you said.
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/i... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/17 at 11:35:06, changseok wrote: > On 2015/03/17 07:42:10, dstockwell wrote: > > On 2015/03/17 at 07:12:58, changseok wrote: > > > On 2015/03/16 21:49:31, dstockwell wrote: > > > > Why can't we interpolate smoothly from 0 to 1.2? > > > > > > Because '0' for font-size-adjust is same meaning with 'none'. This is > > following behavior to the gecko. It treats them as a same value and It doesn't > > animate for 0 nor none if any frame has the values for font-size-adjust. My > > first patch allowed font-size-adjust for '0' to animate though, I changed it not > > to animate after https://codereview.chromium.org/983073002/#msg11. > > > > This sounds like a bug in gecko, we should clarify the behavior with the CSS > > working group before blindly following -- given gecko is the only other > > implementation. > O.K > > > If we must treat 0 the same as none, you can use > > AnimatableDouble::InterpolationIsNonContinuousWithZero in AnimatedStyleBuilder > > and forceDefaultStyleInterpolation in StringKeyframe to prevent interpolating > > other numbers with 0. > I don't have a strong opinion whether or not we should treat '0' as none. But if it should be, I'd like to cover it in a seperated bug later. > For now, let's treat 0 as 'none' as gecko does. OK, but please file the bug and start the discussion with the CSS Working Group before landing this patch. https://codereview.chromium.org/983073002/diff/100001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/983073002/diff/100001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:220: if (AnimatableValue::usesDefaultInterpolation( Hmm, instead of adding these checks here, we can just fall back to the legacy path for the time being: case CSSPropertyFontSizeAdjust: // FIXME: Requires special handing for 0. fallBackToLegacy = true; break;
> OK, but please file the bug and start the discussion with the CSS Working Group before landing this patch. Yup. I created a bug http://crbug.com/468236 . And I emailed www-style@w3.org though it seems delayed to be posted my question since that's my first mail to them. :P https://codereview.chromium.org/983073002/diff/100001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/983073002/diff/100001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:220: if (AnimatableValue::usesDefaultInterpolation( On 2015/03/17 23:36:46, dstockwell wrote: > Hmm, instead of adding these checks here, we can just fall back to the legacy > path for the time being: > > case CSSPropertyFontSizeAdjust: > // FIXME: Requires special handing for 0. > fallBackToLegacy = true; > break; Done.
lgtm On 2015/03/18 at 09:05:03, shivamidow wrote: > > OK, but please file the bug and start the discussion with the CSS Working Group before landing this patch. > Yup. I created a bug http://crbug.com/468236 . And I emailed www-style@w3.org though it seems delayed to be posted my question since that's my first mail to them. :P Great, thanks! https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:65: {at: -0.3, is: 'none'}, This illustrates one of the issues of treating 0 as 'none'. An animation like this would have the text suddenly appear/disappear as the value reaches 0. We could work around this by clamping with a value slightly greater than 0, but I'm OK landing it this way for now.
Thanks all. =) https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/... File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/... LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:65: {at: -0.3, is: 'none'}, On 2015/03/18 09:29:41, dstockwell wrote: > This illustrates one of the issues of treating 0 as 'none'. An animation like > this would have the text suddenly appear/disappear as the value reaches 0. We > could work around this by clamping with a value slightly greater than 0, but I'm > OK landing it this way for now. Noted. Let's handle it in crbug.com/468236 as well. Thanks!
The CQ bit was checked by shivamidow@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/55530)
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from dstockwell@chromium.org Link to the patchset: https://codereview.chromium.org/983073002/#ps160001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/160001
The CQ bit was unchecked by shivamidow@gmail.com
Well I got it why bots are unhappy with this cl. > failures: > tables/mozilla/bugs/bug2886-2.html I once rebased the expected in the first patch https://codereview.chromium.org/943463002. But it re-rebased afterward here https://src.chromium.org/viewvc/blink?view=revision&revision=13315 So the cl needs to add NeedsRebaseline to TestExpectations for the test again. :\
On 2015/03/18 17:02:40, changseok wrote: > Well I got it why bots are unhappy with this cl. > > failures: > > tables/mozilla/bugs/bug2886-2.html > I once rebased the expected in the first patch > https://codereview.chromium.org/943463002. But it re-rebased afterward here > https://src.chromium.org/viewvc/blink?view=revision&revision=13315 > So the cl needs to add NeedsRebaseline to TestExpectations for the test again. > :\ No. my bad. previous result was just not perfect.
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from dstockwell@chromium.org Link to the patchset: https://codereview.chromium.org/983073002/#ps160002 (title: "Rebase expectations for bug2886-2.html")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/160002
Message was sent while issue was closed.
Committed patchset #10 (id:160002) as https://src.chromium.org/viewvc/blink?view=rev&revision=192116 |
