Bug 122524 – [CSS Shapes] Shape-Margin should be animatable

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

Changing shape-margin dynamically should cause relayout.
Shape-margin should also be animatable via CSS.

[Bear Travis]

Created attachment 213720 [details]
Initial patch

[Darin Adler]

Comment on attachment 213720 [details]
Initial patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213720&action=review

> Source/WebCore/ChangeLog:3155
> +        [CSS Shapes] Basic shapes should be animatable for shape-outside
> +        https://bugs.webkit.org/show_bug.cgi?id=122343
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/shapes/shape-outside-floats/shape-outside-animation.html
> +
> +        Add shape outside to the list of animatable properties. The infrastructure
> +        is already in place for animating basic shapes on shape-inside and clipping
> +        paths. See https://bugs.webkit.org/show_bug.cgi?id=101854.
> +
> +        * page/animation/CSSPropertyAnimation.cpp:
> +        (WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):

Extra change log here. Oops!

> Source/WebCore/rendering/RenderBox.cpp:391
>      // FIXME: A future optimization would do a deep comparison for equality. (bug 100811)

This comment makes more sense just before the "==". It’s a little strange to have it paragraphed with code that just sets up shapeOutside locals.

> Source/WebCore/rendering/RenderBox.cpp:393
> +    const ShapeValue* oldShapeOutside = oldStyle ? oldStyle->shapeOutside() : 0;

Please use nullptr instead of 0.

> Source/WebCore/rendering/RenderBox.cpp:396
> +    const Length shapeMargin = style->shapeMargin();
> +    const Length oldShapeMargin = oldStyle ? oldStyle->shapeMargin() : RenderStyle::initialShapeMargin();

The const don’t add much value here.

> Source/WebCore/rendering/RenderBox.cpp:405
>          ShapeOutsideInfo* shapeOutsideInfo = ShapeOutsideInfo::ensureInfo(this);
>          shapeOutsideInfo->dirtyShapeSize();

This would read better without the local variable.

Also, is it really necessary to create the info with an “ensure” function just to dirty the size? Seems that if there was no info there is nothing to dirty.

> Source/WebCore/rendering/RenderBox.h:645
> -    void updateShapeOutsideInfoAfterStyleChange(const ShapeValue* shapeOutside, const ShapeValue* oldShapeOutside);
> +    void updateShapeOutsideInfoAfterStyleChange(const RenderStyle*, const RenderStyle* oldStyle);

First argument should be const RenderStyle& since it’s guaranteed to never be null.

> LayoutTests/ChangeLog:733
> +        [CSS Shapes] Basic shapes should be animatable for shape-outside
> +        https://bugs.webkit.org/show_bug.cgi?id=122343
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add tests checking that shape-outside basic shape values correctly tween
> +        between values.
> +
> +        * animations/resources/animation-test-helpers.js:
> +        (getPropertyValue): Add shape-outside to list of properties that do not parse
> +        with the default behavior.
> +        (comparePropertyValue): Compare shape-outsides after parsing their shape notation.
> +        * fast/shapes/shape-outside-floats/shape-outside-animation-expected.txt: Added.
> +        * fast/shapes/shape-outside-floats/shape-outside-animation.html: Added.

Extra change log here.

[Bear Travis]

Created attachment 213819 [details]
Incorporating feedback

Making suggested changes.
The ensureInfo is called because there may be a ShapeInfo we are going to reuse, in which case we need to dirty the size bit.

[Bear Travis]

Created attachment 214026 [details]
Updated patch

Merging to master/trunk

[WebKit Commit Bot]

Comment on attachment 214026 [details]
Updated patch

Clearing flags on attachment: 214026

Committed r157400: <http://trac.webkit.org/changeset/157400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.