Bug 119938 – Animate CSS Image filter() function

• Component: CSS
• Status: RESOLVED FIXED

[Dirk Schulze]

WebKit should support animating filter().

[Dirk Schulze]

Created attachment 208984 [details]
Patch

[WebKit Commit Bot]

Attachment 208984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/animations/resources/animation-test-helpers.js', u'LayoutTests/fast/filter-image/filter-image-animation-expected.txt', u'LayoutTests/fast/filter-image/filter-image-animation.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.h', u'Source/WebCore/css/CSSFilterImageValue.h', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp']" exit_code: 1
Source/WebCore/page/animation/CSSPropertyAnimation.cpp:221:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Created attachment 208985 [details]
Patch

[Dirk Schulze]

Created attachment 209007 [details]
Patch

Prepare CSSPropertyAnimation for future generated image blend operations.

[EFL EWS Bot]

Comment on attachment 209007 [details]
Patch

Attachment 209007 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1497206

[Early Warning System Bot]

Comment on attachment 209007 [details]
Patch

Attachment 209007 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1387361

[Early Warning System Bot]

Comment on attachment 209007 [details]
Patch

Attachment 209007 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1433645

[kov's GTK+ EWS bot]

Comment on attachment 209007 [details]
Patch

Attachment 209007 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1433646

[EFL EWS Bot]

Comment on attachment 209007 [details]
Patch

Attachment 209007 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1387364

[Dirk Schulze]

Created attachment 209013 [details]
Patch

patch with build fix

[Dirk Schulze]

Created attachment 209079 [details]
Patch

Add toCSSFilterImageValue functions as suggested by anttik.

[Antti Koivisto]

Comment on attachment 209079 [details]
Patch

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

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:217
> +    CSSFilterImageValue* fromValue = static_cast<CSSFilterImageValue*>(from->data());
> +    CSSFilterImageValue* toValue = static_cast<CSSFilterImageValue*>(to->data());

You should use you casting functions here too.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:321
> +        CSSImageGeneratorValue* fromGenerated = static_cast<CSSImageGeneratorValue*>(from->data());
> +        CSSImageGeneratorValue* toGenerated = static_cast<CSSImageGeneratorValue*>(to->data());

The could have casting functions.

[Antti Koivisto]

Not my best typing day.

[Simon Fraser (smfr)]

Comment on attachment 209079 [details]
Patch

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

Looks OK but I'd like to see a patch with the enum and other comments addressed.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:65
> +    static PassRefPtr<CSSValue> valueForFilter(const RenderObject*, const RenderStyle*, const FilterOperations&, bool adjust = true);

It's not clear what "adjust" means in this context. You should use an enum.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:80
> -    PassRefPtr<CSSValue> valueForShadow(const ShadowData*, CSSPropertyID, const RenderStyle*) const;
> +    static PassRefPtr<CSSValue> valueForShadow(const ShadowData*, CSSPropertyID, const RenderStyle*, bool adjust = true);

It's not clear what "adjust" means in this context. You should use an enum.

> Source/WebCore/css/CSSFilterImageValue.h:73
> +    bool equalSubimages(const CSSFilterImageValue&) const;

subimage is confusing; we should rename this ("input image"?)

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:225
> +    blendFilterOperations(
> +        anim,
> +        filterOperationsResult,
> +        fromValue->filterOperations(),
> +        toValue->filterOperations(),
> +        progress);

This doesn't need to be so aggressively wrapped.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> +    RefPtr<CSSValue> filterValue = ComputedStyleExtractor::valueForFilter(
> +        anim->renderer(),
> +        anim->renderer()->style(),
> +        filterOperationsResult,
> +        false);

Ditto. 
Congratulations, you just fell into the boolean trap.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:323
> +        ASSERT(fromGenerated);
> +        ASSERT(toGenerated);

Not sure these assertions are useful, since (if filters are enabled) you'll crash on the next line anyway.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:327
> +            // Animation of generated images just possible if subimages are equal.

s/just/is only
I don't really know what a subimage is in this context. It seems different from our use of the term in other places.

[Dirk Schulze]

Created attachment 209116 [details]
Patch

Addresses anttik's and smfr's feedback.

[Simon Fraser (smfr)]

Comment on attachment 209116 [details]
Patch

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

> LayoutTests/fast/filter-image/filter-image-animation.html:58
> +    @-webkit-keyframes no-anim {
> +        from { background-image: -webkit-filter(url(resources/image.svg), sepia(1)); }
> +        to   { background-image: -webkit-filter(url(resources/blue.svg), sepia(0)); }
> +    }

You should test a blend between a filter and simple image, and a filter and gradient, and filter and crossfade (to check for code that behaves badly).

You should also test for blends between filter lists of the same and different lengths, and maybe with missing filter lists.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:903
> +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(double length, const RenderStyle* style, ZoomAdjustedPixel adjust)

This has a very confusing name; it sounds like you're lookup up the RGBA for a single pixel.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:908
> +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(const Length& length, const RenderStyle* style, ZoomAdjustedPixel adjust)

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:51
> +enum ZoomAdjustedPixel { AdjustPixel, DoNotAdjustPixel };

Enum types should be descriptive, like AdjustPixelVaulesForZoom { AdjustPixelValuesForZoom, DoNotAdjustPixelValues }

> Source/WebCore/css/CSSFilterImageValue.cpp:168
> +bool CSSFilterImageValue::equalInputImages(const CSSFilterImageValue& other) const

Much better than subimage!

> Source/WebCore/css/CSSFilterImageValue.h:49
> +

Blank line.

[Dirk Schulze]

Created attachment 209953 [details]
Patch

[Build Bot]

Comment on attachment 209953 [details]
Patch

Attachment 209953 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1640119

New failing tests:
animations/cross-fade-webkit-mask-box-image.html
animations/cross-fade-webkit-mask-image.html
animations/cross-fade-background-image.html
animations/cross-fade-border-image-source.html
animations/cross-fade-list-style-image.html

[Build Bot]

Created attachment 209962 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209953 [details]
Patch

Attachment 209953 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1627569

New failing tests:
animations/cross-fade-webkit-mask-box-image.html
animations/cross-fade-webkit-mask-image.html
animations/cross-fade-background-image.html
animations/cross-fade-border-image-source.html
animations/cross-fade-list-style-image.html

[Build Bot]

Created attachment 209967 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Dirk Schulze]

Created attachment 209973 [details]
Patch

[Build Bot]

Comment on attachment 209973 [details]
Patch

Attachment 209973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1640183

New failing tests:
animations/cross-fade-border-image-source.html

[Build Bot]

Created attachment 209978 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209973 [details]
Patch

Attachment 209973 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1605646

New failing tests:
animations/cross-fade-border-image-source.html

[Build Bot]

Created attachment 209994 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Simon Fraser (smfr)]

Comment on attachment 209973 [details]
Patch

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

You should add a test for animations between -webkit-filter(url(foo), ...) and url(foo), which should animate the filter values to the no-op filter (just like the filter property).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:903
> +static inline PassRefPtr<CSSPrimitiveValue> adjustLengthForZoom(double length, const RenderStyle* style, AdjustPixelVaulesForZoom adjust)

AdjustPixelVaulesForZoom is mis-spelled. It's also weird to have the same name for the enum as one of its values.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:919
> +    for (const ShadowData* s = shadow; s; s = s->next()) {

We don't use short variable names like 's'. Maybe currShadowData?

> Source/WebCore/css/CSSComputedStyleDeclaration.h:51
> +enum AdjustPixelVaulesForZoom { AdjustPixelValuesForZoom, DoNotAdjustPixelValues };

Spelling.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:221
> +    blendFilterOperations(anim, filterOperationsResult,
> +        fromValue->filterOperations(), toValue->filterOperations(), progress);

I don't think you need to wrap these lines.

[Dirk Schulze]

Created attachment 210016 [details]
Patch

[Dirk Schulze]

Created attachment 210034 [details]
Patch

[Dirk Schulze]

Created attachment 210035 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210035 [details]
Patch

Clearing flags on attachment: 210035

Committed r154906: <http://trac.webkit.org/changeset/154906>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.