Bug 21725 – Should be able to do transitions of background-image, including gradients

• Component: CSS
• Status: NEW

[paradox]

The "transition-property" function does not seem to support changes in background-image, or any other type of background change other than color. the "all" parameter does not affect backgrounds either.

While not a major block, this is hobbling to such a useful function.

I was intending on using this in a list based navigation bar, to generate a transition between normal and hover states.

[Dave Hyatt]

What kind of transition do you want to see between two images?  A dissolve (opacity blend between the two)?  We had talked about doing that and possibly supporting other kinds of transitions as well.

[paradox]

(In reply to comment #1)
> What kind of transition do you want to see between two images?  A dissolve
> (opacity blend between the two)?  We had talked about doing that and possibly
> supporting other kinds of transitions as well.
> 

A dissolve was exactly what i was thinking of.

[Faruk Ates]

A dissolve would be great; other kinds of transitions would be truly Awesome™.

For me personally, I'd really like to see background-position be Transitioned as well. Should that be a separate bug report or is this one ambiguously enough defined to be used for that, too?

[Simon Fraser (smfr)]

Please file a separate bug for background-position.

[Simon Fraser (smfr)]

Bug 23984 filed on transitions of background-position.

[Simon Fraser (smfr)]

*** Bug 26530 has been marked as a duplicate of this bug. ***

[Lea Verou]

Gradient and image interpolation is clearly defined in the Image Values and Replaced Content Module Level 3 (http://www.w3.org/TR/css3-images/#interpolating-gradients) spec, so this looks like a spec violation, not just an inconvenient shortcoming.

[Simon Fraser (smfr)]

Calling it a spec violation is a bit strong. That spec stuff is very new and no-one has implemented it yet.

[Lea Verou]

Technically, everything in the spec that's not implemented could be considered a spec violation, right? :-)
But yeah, you're right it was a bit strong, sorry.

[Shane Stephens]

I'm interested in taking a look at this, starting with gradients.

[Simon Fraser (smfr)]

You shouldn't start with gradients. You should start with simple cross-fades of images.

I think you need a generated image type which has two input images, and draws a cross-faded in-between image.

[Faruk Ates]

Spec violation -> something that is implemented in a directly conflicting way to how the spec defines it.
Spec omission -> something that is not yet implemented at all according to the spec.

[Shane Stephens]

OK, I'll start with cross-fade then.  I thought gradients would be simpler?

[Simon Fraser (smfr)]

Image cross-fade will force you to set up the right infrastructure.

[Dean Jackson]

I suggested Shane start with gradients because I thought it would be fairly simple to make a Blend that operates between the gradient settings. Any gradients that do not match would automatically fall back onto the cross fade image background case.

I guess it is a choice between addressing the easier but obscure case and the more difficult but general case.

[Dean Jackson]

e.g

1. add a new case to FillLayersPropertyWrapper
2. implement the new case as FillLayerPropertyWrapper<StyleImage>(&FillLayer::image, &FillLayer::setImage)
3. check if they are StyleGeneratedImage (for gradient)
4. get the generator (CSSImageGeneratorValue)

At this point you'll need some way to detect if the CSSImageGeneratorValue is a CSSGradientValue

Then check the types, repeating, num stops. Lastly, iterate the stops to do the blend.

[Dean Jackson]

BTW - what type of crossfade do we want?

If we move src opacity from 1 to 0 while dst opacity from 0 to 1, then we'll have a flash of the page background in the middle. This often looks terrible.

If we keep src opacity at 1 and ramp dst from 0 to 1, then we avoid the flash but it will look terrible if the dst has any transparent regions (the src will disappear at the end).

Should we fade through the background color if there is one? Not really, since that is a separate property.

Typically people cross fade photos using the 2nd option, because photos have no transparent regions. The first option will give a washed out image during the transition.

[Faruk Ates]

This is where I think several CSS specs are woefully incomplete in terms of what they could permit, implementation-wise, for a scenario like this one. 

To me, fade through background-color seems like the only logical way to do that *if* that is what you want to achieve. And it's a legitimate use case, so I can see people wanting to do this. If you *don't* fade through the background-color (if one is set), how else would you make it happen? You'd need to start setting explicit transition properties for each individual background image, but I don't think that this is currently syntactically possible with the multiple backgrounds & transitions implementations.

I don't even know what to feasibly expect from this anyway, given multiple backgrounds. On a surface level, it seems relatively straightforward. Three principal options:

- src url() swapped with new dest url()
- src url() preceded by new dest url()
- src url() followed by new dest url()

Then, what do I do if I want this:

- url(A) swapped with new url(D) over 1s linear
- url(B) swapped with new url(E) over .5s ease-out
- url(C) swapped with new url(F) over .75s ease-in

This could syntactically be done using the / separator, though I'm not sure this following example is really such a great idea:

foo {
  background-image: url(A), url(B), url(C);
  margin-left: 100px;
  transition: background-image 1s linear / .5s ease-out / .75s ease-in, margin-left 1s ease-out;
}

foo.new {
  background-image: url(D), url(E), url(F);
  margin-left: 0;
}

But what can I do for this kind of (to me legitimate) scenario?

- url(A) swapped with new url(D) over 1s linear, cross-dissolving src 1 to 0 to dest 0 to 1
- url(B) swapped with new url(E) over .5s ease-out, cross-dissolving dest 0 to 1 "on top of" src
- url(C) swapped with new url(F) over .75s ease-in fading through the background-color 

background-nth-layer(n) or something like that probably wouldn't have had this issue. ;-)
(I don't know if that was ever a proposal)

[Simon Fraser (smfr)]

Can of worms is officially open.

[Tab Atkins]

Targetting individual entries in a list-valued property will appear for free  when we get the ability to replace individual entries in the cascade.  I'd strongly recommend against trying to solve it here for Transitions specifically.  For now, just let list-valued properties transition all their entries with the same transition parameters.

[Dean Jackson]

I'm not sure we should fade through background color. Background color is a separate property, and will be rendered regardless of what is happening to the background image. You might not be able to see it, but it should be there. The issue is that most times it is transparent.

I think the two big problems are:

- if you do src 1->0 while dst 0->1 it usually looks bad in the middle. People won't like it.
- if you do src 1 while dst 0->1 it will look very bad unless dst has no transparent regions.

Of course, this is all ok with gradients that match type and number of stops.

[Shane Stephens]

I agree with Tab that we should let all entries in a list-valued property transition with the same parameters.  Given that, playing with the actual transition itself should be relatively simple once we have the mechanism in place.

I'd like to suggest that I just get something working (maybe the src 1->0 dest 0->1) and then we can iterate on that?

[Tab Atkins]

(In reply to comment #22)
> I'd like to suggest that I just get something working (maybe the src 1->0 dest 0->1) and then we can iterate on that?

I don't have a lot of graphics experience, but I've done a little bit of research now, and it looks like using the 'add' compositing operator (rather than src-over) would be a good start.  Just ramp the alphas as you described (src 1=>0, dst 0=>1), and then composite by adding the premultiplied colors and alphas, clamping when necessary.  Is this operator supported by the platform libraries?

According to the SVG Compositing spec, this operation is commonly used for blending images, and I see why.  You avoid the "two opaque images become partially-transparent part-way through the transition" problem, while still working well for partially-transparent images.

[Shane Stephens]

Created attachment 92913 [details]
Patch

[Shane Stephens]

This patch is a work-in-progress.  It implements non-repeating linear and radial gradient transitions (i.e. repeating gradients and general background images are not yet handled). I'm uploading it so I can get style and implementation comments early, before implementing all of the different cases.

A layout test is included in the patch - the test passes when viewed in Safari, but not through the layout test framework.  Here's why: gradient interpolation is required by the spec to occur over "used" (or screen) values, and hence the correct value for an interpolated gradient can only be determined during a render.  However, the transition testing framework examines objects' computedStyle to determine their interpolated values without necessarily rendering first.

Apparently there's a new animation API coming that will allow us to force renders before checking computedStyle, which will solve this problem. If anyone has any other ideas for how to get around the issue without waiting for this API then that'd be cool too :)

[WebKit Review Bot]

Comment on attachment 92913 [details]
Patch

Attachment 92913 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8653836

New failing tests:
transitions/mask-transitions.html
transitions/clip-transition.html
transitions/shorthand-border-transitions.html
transitions/color-transition-all.html
transitions/shorthand-transitions.html
transitions/svg-text-shadow-transition.html
transitions/mismatched-shadow-styles.html
transitions/multiple-mask-transitions.html
transitions/multiple-background-transitions.html
transitions/background-transitions.html
transitions/multiple-background-size-transitions.html
transitions/multiple-shadow-transitions.html
transitions/color-transition-premultiplied.html

[WebKit Review Bot]

Created attachment 92914 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Simon Fraser (smfr)]

I don't get why you need to add classes like CSSInterpolatedRadialGradientValue to the CSSOM. Existing transitions/animations act on RenderStyle-level objects (and hence would affect StyleImage and friends). Also, every intermediate state should be representable using the existing CSSCOM classes, otherwise the endpoints would not be interpolatable.

There is an issue that StyleGeneratedImage references the CSSImageGeneratorValue; you need to break this connection, and store gradient data in a Style-level class in order to render the intermediate states.

BTW, I still think starting with image cross-fade is better.

[Shane Stephens]

From the spec (http://dev.w3.org/csswg/css3-images/#interpolating-gradients), gradients must be converted to their "used values" before interpolating.  In order to do so, the rendered size of the gradient must be known - e.g. linear-gradient(red 200px, white 50%) has used values for stops of 100%, 100% at a rendered height of 200px, 50%, 50% at 400px, and 40%, 50% at 500px. Furthermore, without converting to a consistent coordinate space, how would a transition from a stop at 40% to a stop at 300px work?

The rendered size is only available at render time and is not inferable from the CSSOM representation of the gradient background.  Hence interpolation calculations have to be deferred to render time, which is the purpose of CSSInterpolatedLinearGradient and CSSInterpolatedRadialGradient.

The intermediate states are representable using CSSLinearGradient and CSSRadialGradient but not calculable until the rendered dimensions are known.

[Dean Jackson]

Comment on attachment 92913 [details]
Patch

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

This has been sitting here a while. I think we should move forward or give reasons why not.

I agree with Simon that the classes duplicating the gradient data are unfortunate, but if that's the only way.. oh well.

> Source/WebCore/css/CSSGradientValue.cpp:568
>  

Without following this code through, I wonder if it would create an animation that goes the long way around the circle. Say from 0 -> 315 degrees. Of course, that is probably the way it is described in the specification, and hence formally expected behaviour.

> Source/WebCore/css/CSSGradientValue.cpp:587
> +            if (angle < 0)

I don't really mind either way, but it might be more clear to have a boolean angleIsSet or something, rather than relying on the -1 value above. The !angle vs != 0 got me below as well. It's a tricky combination of logic in the entire method.

> Source/WebCore/page/animation/AnimationBase.cpp:307
> +                blendFunc(0, fromStop.stop, toStop.stop, m_amount), blendFunc(0, fromStop.red, toStop.red, m_amount),

style nit - i think this would read better each on one line if you're doing manual wrapping anyway.

> Source/WebCore/page/animation/AnimationBase.cpp:325
> +            const Gradient::ColorStop stop = m_gradient->getStops()[i];

Local variable for m_gradient->getStops() ? You reference it twice every iteration.

> Source/WebCore/page/animation/AnimationBase.cpp:326
> +            result += ", ";

See the webkit-dev thread about building strings using StringBuilder rather than +=

> Source/WebCore/page/animation/AnimationBase.cpp:349
> +    : CSSRadialGradientValue(from->isRepeating() ? Repeating : NonRepeating)

Should this be indented?

> Source/WebCore/page/animation/AnimationBase.cpp:386
> +                                                     blendFunc(0, fromStop.green, toStop.green, m_amount), blendFunc(0, fromStop.blue, toStop.blue, m_amount),

ditto above

> Source/WebCore/page/animation/AnimationBase.cpp:401
> +        for (unsigned i = 0; i < m_gradient->getStops().size(); i++) {

ditto above

> Source/WebCore/page/animation/AnimationBase.cpp:403
> +            result += ", ";

ditto above

> Source/WebCore/page/animation/AnimationBase.cpp:470
> +                    result.get()->ref();

pardon my ignorance, but why do you have to do this? where is the code that decrements the reference? Why can't you pass in result (and image below)?

> Source/WebCore/platform/graphics/Gradient.cpp:203
> +    sortStopsIfNecessary();

It's weird that there is sortStopsIfNeeded() and sortStopsIfNecessary() on similar classes. I guess here isn't the place to fix that.

[Dean Jackson]

Obviously I'm not skilled at using the review tool :(

Sorry that the comments have almost no context.

[Simon Fraser (smfr)]

(In reply to comment #29)
> From the spec (http://dev.w3.org/csswg/css3-images/#interpolating-gradients), gradients must be converted to their "used values" before interpolating.  In order to do so, the rendered size of the gradient must be known - e.g. linear-gradient(red 200px, white 50%) has used values for stops of 100%, 100% at a rendered height of 200px, 50%, 50% at 400px, and 40%, 50% at 500px. Furthermore, without converting to a consistent coordinate space, how would a transition from a stop at 40% to a stop at 300px work?
> 
> The rendered size is only available at render time and is not inferable from the CSSOM representation of the gradient background.  Hence interpolation calculations have to be deferred to render time, which is the purpose of CSSInterpolatedLinearGradient and CSSInterpolatedRadialGradient.

Right, but these should not inherit from CSSValue; they should live in the Render* world.

We blur the line between RenderStyle-type data, and CSSValue data for gradients, and that's a mistake. Ideally, we'd never have to go back to CSSValue things when painting (or animating).

[Shane Stephens]

After a chat on irc with smfr about this, he and I agreed that the specification needs adjusting, as currently it's not possible to generate intermediate gradients without knowing the rendered size.

The adjustments we are going with for now are (1) only allowing interpolation from percentages to percentages or from pixel values to pixel values, and (2) performing linear interpolation *before* adjusting the stops to take into account potential overlaps.

What the first condition means in concrete terms is that you can't e.g. transition between:

-linear-gradient(red: 200px, white 50%)
and
-linear-gradient(red: 60%, white: 400px)


The second means that an animation between

-linear-gradient(red: 200px, white 50%)
and
-linear-gradient(red: 100px, white 60%)

will do different things at a rendered size of 400px and at a rendered size of 300px.

At 400px, the red 200px and white 50% coincide in the start gradient and both will linearly interpolate to their finishing values.

At 300px, red will be adjusted to 150px in the start gradient, but as interpolation occurs before adjustment, the red stop will move to the right for a short time before moving left (e.g. at 20%, red will be 180px and white will be 52%, which will resolve to 15px, so red will be adjusted post-interpolation to 156px; but at 50%, red will be 150px and white will be 55%, which will resolve to 165px, so no adjustment will occur).

[Dean Jackson]

Sounds good!

[Tab Atkins]

(In reply to comment #33)
> After a chat on irc with smfr about this, he and I agreed that the specification needs adjusting, as currently it's not possible to generate intermediate gradients without knowing the rendered size.
> 
> The adjustments we are going with for now are (1) only allowing interpolation from percentages to percentages or from pixel values to pixel values, and (2) performing linear interpolation *before* adjusting the stops to take into account potential overlaps.

This is *much* more restrictive than you're letting on.  It also means, for example, that it's impossible transition to or from a "corner-to-corner" gradient, as they require the rendered size to determine their effective angle.

Similarly, it makes elliptical gradients impossible to transition between unless they both have the same size/shape keywords, or both use explicit sizing with the same units.

In general terms, this is the "I want to transition between height:0 and height:auto" problem, which we *need* to solve because it's very useful.  Just punting because it's hard isn't acceptable.  If we can come up with a good solution for gradients, hopefully we can generalize that to working for other properties/values as well.


I'm not opposed to rearranging things such that stop-adjustment happens at the last possible moment, and interpolation occurs before that.

[Simon Fraser (smfr)]

(In reply to comment #35)
> (In reply to comment #33)
> In general terms, this is the "I want to transition between height:0 and height:auto" problem, which we *need* to solve because it's very useful.  Just punting because it's hard isn't acceptable.  If we can come up with a good solution for gradients, hopefully we can generalize that to working for other properties/values as well.

It is similar, but it also reflects the lack of a single canonical form for endpoint and angle gradients. Maybe we can convert everything into the angle form internally.

[Tab Atkins]

(In reply to comment #36)
> It is similar, but it also reflects the lack of a single canonical form for endpoint and angle gradients. Maybe we can convert everything into the angle form internally.

That's not possible, unfortunately, as you want to always do a "shortest path" transition when going keyword->keyword.  Specifically, if you go from bottom->right, you want a quarter-turn rotation, rather than a 3/4-turn from 0deg to 270deg.  This was specifically raised as a spec bug by Microsoft.  (If you mix keywords and angles, nothing special happens - doing bottom->270deg does a 3/4-turn rotation.)

In all other circumstances, though, it works fine to just convert the keyword into an angle.  If you kept a flag around to indicate that the angle came from a keyword, you could then do an easy check at animation time to see if you need to treat one of the angles as being +360deg or not.

This could be avoided by my preferred resolution to the "keywords are confusing" problem (replacing the keywords with an "<angle> snap?" form).  You still need layout information to convert a snapped angle into an absolute angle, but once you've done so you can just work with that angle for everything else.

[Shane Stephens]

Created attachment 103447 [details]
Patch

[Shane Stephens]

I've just uploaded a proof-of-concept patch that creates a canonical form of gradients at applyProperty time where possible, then animates between canonical forms where they exist - i.e. the approach that smfr has been recommending.

This approach has some limitations that stem from the situations when canonical forms do not exist, as well as some limitations that stem from the fact that the canonical form of the gradient does not completely specify the view of the gradient on the screen.

The canonical form does not exist when the rendered size of the gradient is required to describe the gradient canonically, as this information is not available at applyProperty time. The situations in which this is true are:
(1) when the corner keywords are used to specify a linear gradient angle (top left, top right, etc.): the angle can't be determined until the aspect ratio of the container is known.
(2) when the implicit keywords are used to specify a radial gradient location (closest-side, etc.): these values can not in general be determined until the placement of the gradient in the box is known.

The canonical form does not completely specify the view of the gradient because:
(1) the length of the gradient relative to the size of the container can't be determined until the aspect ratio of the container is known. This means that we can't do smooth rotations (rotations where the control points follow the box edge are OK).
(2) the current specification requires position resolution (including shifting of occluded stops) to occur before distribution of stops without locations. This is not possible as shifting of occluded stops can't occur without knowing the size of the box, in order to resolve pixel-specified stops against percent-specified stops.

Additionally, with this approach a gradient cssText in general can't be generated without caching the renderable Gradient object.

So, thoughts? Which approach is better? This one breaks the spec slightly and limits the cases in which a transition can apply, but is cleaner in terms of the existing layers in webkit. The former approach can implement the specified behaviour fully and accurately, but requires blurring of these layers.

[Tab Atkins]

(In reply to comment #39)
> So, thoughts? Which approach is better? This one breaks the spec slightly and limits the cases in which a transition can apply, but is cleaner in terms of the existing layers in webkit. The former approach can implement the specified behaviour fully and accurately, but requires blurring of these layers.

It's more than a slight breaking.  It means that you can't transition between linear gradients if either endpoint is corner-to-corner, or between radial gradients if either endpoint uses keyword-based sizing.  The WG resolved today to add logical keywords to the linear-gradient positioning argument (suggested by smfr), which I believe also suffers from this problem, as the element's directionality may not be computable until used-value time (for example, if it's direction:auto).

Like I said in an earlier comment, transitions based on used values are absolutely necessary (the height:0 to height:auto problem).  Trying to hack around this problem in gradients by greatly restricting what is possible to transition isn't the right answer.

[Tab Atkins]

(In reply to comment #40)
> Like I said in an earlier comment, transitions based on used values are absolutely necessary (the height:0 to height:auto problem).  Trying to hack around this problem in gradients by greatly restricting what is possible to transition isn't the right answer.

(That said, I'm okay with a partial implementation now, followed by a full implementation later.)

[Dean Jackson]

Me too. I think it is ok for this to land in an incomplete state, provided we don't predict we'll change the behaviour of what does land.

[Tab Atkins]

(In reply to comment #42)
> Me too. I think it is ok for this to land in an incomplete state, provided we don't predict we'll change the behaviour of what does land.

Yeah, the patch properly addresses the behavior for the cases it chooses to handle.

[Shane Stephens]

tl;dr: we need to choose between:
(1) possibly mildly breaking the web by changing the output rendering for some strangely-specified gradients
(2) preventing animations occurring for hard-to-define-neatly categories of gradients
(3) preventing animations occurring unless gradients match up exactly in stop type
(4) animations with sudden changes in directions under certain circumstances.

Messy details below.

There is one case where this patch changes current behaviour. The spec indicates the following ordering for resolution of stop positions:

(1) convert explicit stops to percentage value
(2) adjust explicit stops forward if they lie before a previous value
(3) evenly space remaining implicit stops in the space allocated to them

for example:

red 40px, white 10%, green, blue, black 100%
for a size of 100px:

red -> 40px
white -> 10px (1) -> 40px (2)
black -> 100px (1)
green, blue -> 60px, 80px (3)

However this isn't possible with the intermediate representation I've chosen, which requires explicit positions for all stops as a combination of percent + value. I chose this representation as without it we can't animate between implicitly placed stops and explicitly placed stops.

What I do is switch (2) and (3), as (2) can only occur at render time. This results in a different gradient if there's at least one each of percentage- and pixel- stops before some implicit stops, as in the example above. What will happen now is:

red -> 40px 0%
white -> 0px 10%
black -> 0px 100%
green, blue -> 0px 40%, 0px 70%

then at render time

red -> 40px
white -> 10px -> 40px
green -> 40px
blue -> 70px
black -> 100px

So the patch as it stands changes behaviour in these specific cases.

There are at least 3 alternatives:

(1) punt on these cases as they're detectable. What this means in practice is that animations will simply fail to work for no super-obvious reason (e.g. how do you indicate what's wrong with the specification above without delving deep into implementation?)
(2) Only allow animations in cases where percent stops match percent stops, pixel stops match pixel stops, and implicit stops match implicit stops. This is probably the sanest approach but it cuts out a large number of animations, again for an implementation detail.
(3) Store a flag with each stop that indicates whether it's implicit or not and defer stop resolution to render time. This requires the ability to have stops in the data representation which are "partially implicit" (e.g. halfway between an implicit position and an explicit position) which is .. interesting, and will also result in some curious effects such as sudden changes in direction when animating container bounds at the same time, but it may be the best approach overall.

I'm going to spend a bit of time putting together a javascript demo of approach 3 to see what it looks like. I'll upload it to the bug when done.

[Tab Atkins]

(In reply to comment #44)
> There are at least 3 alternatives:

4) I change the spec to swap steps 2 and 3.  Either ordering "makes sense" and still results in smooth animations.

[Shane Stephens]

Created attachment 105465 [details]
Patch

[Shane Stephens]

This latest version includes an implementation of cssText().

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSGradientValue.cpp:651:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files


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

[WebKit Review Bot]

Comment on attachment 105465 [details]
Patch

Attachment 105465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9549993

[Early Warning System Bot]

Comment on attachment 105465 [details]
Patch

Attachment 105465 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9566030

[Shane Stephens]

Created attachment 106544 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106544 [details]
Patch

Attachment 106544 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9554044

New failing tests:
fast/gradients/css3-linear-angle-gradients.html
fast/gradients/css3-repeating-linear-gradients2.html
fast/gradients/css3-repeating-end-fill.html
fast/gradients/css3-repeating-linear-gradients.html
fast/gradients/css3-color-stops.html
fast/repaint/gradients-em-stops-repaint.html
fast/gradients/css3-radial-gradients3.html

[Dean Jackson]

ping? shane, do you want a review still? what about the failing tests?

[Shane Stephens]

Hi Dean,

Ah sorry, I should mark this version as review-. These failing tests alerted me to some issues with caching of the new layout-independent layer that I'm still (slowly) trying to work through - I think I'm going to need to add RefPtrs through the rendering/style layer. I'll upload another patch when I can.

[Tim Horton]

Created attachment 118159 [details]
[preliminary] Use -webkit-cross-fade to implement background-image (and list-style-image, etc.) transitions

This patch still needs:

a) some more shorthands to work (border-image, for one).
b) tests! I have them, but not in layout test format yet.

but in the interests of getting it going, I'm uploading it now so people can take a look.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/page/animation/AnimationBase.cpp:975:  One space before end of line comments  [whitespace/comments] [5]
LayoutTests/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 9 files


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

[Shane Stephens]

Hi Tim,

I think we're implementing different functionality in the same area (I'm implementing gradient transitions, you're implementing background-image transitions). It probably makes sense to split this bug in two :)

Do you want to create a new bug or shall I?

[Tim Horton]

(In reply to comment #57)
> Hi Tim,
> 
> I think we're implementing different functionality in the same area (I'm implementing gradient transitions, you're implementing background-image transitions). It probably makes sense to split this bug in two :)
> 
> Do you want to create a new bug or shall I?

I was internally debating that earlier, and decided to stick with it because of the title (and the fact that we're going to share a bunch of code in the end). But, you're right, there are too many comments here, and the functionality is separate; I'll attach my next patch (tomorrow) to a new bug.

[Tim Horton]

(In reply to comment #57)
> Hi Tim,
> 
> I think we're implementing different functionality in the same area (I'm implementing gradient transitions, you're implementing background-image transitions). It probably makes sense to split this bug in two :)
> 
> Do you want to create a new bug or shall I?

FYI, my code landed in http://trac.webkit.org/changeset/102388. I left a FIXME in AnimationBase (in the blendFunc that takes StyleImages) for you to plug in your gradient transitioning code.

[Bronislav Klucka]

(In reply to comment #59)
> (In reply to comment #57)
> > Hi Tim,
> > 
> > I think we're implementing different functionality in the same area (I'm implementing gradient transitions, you're implementing background-image transitions). It probably makes sense to split this bug in two :)
> > 
> > Do you want to create a new bug or shall I?
> 
> FYI, my code landed in http://trac.webkit.org/changeset/102388. I left a FIXME in AnimationBase (in the blendFunc that takes StyleImages) for you to plug in your gradient transitioning code.

Will this code be allowed or not?

[Shane Stephens]

Created attachment 133674 [details]
Patch

[Shane Stephens]

As Simon, Dean and I discussed at TPAC, here's a patch that uses the original approach, because interpolating without knowing the render size just can't be made to work.

[Shane Stephens]

Created attachment 133776 [details]
Patch

[Build Bot]

Comment on attachment 133776 [details]
Patch

Attachment 133776 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12133743

[Gyuyoung Kim]

Comment on attachment 133776 [details]
Patch

Attachment 133776 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12127830

[WebKit Review Bot]

Comment on attachment 133776 [details]
Patch

Attachment 133776 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12130864

New failing tests:
transitions/svg-transitions.html
transitions/cross-fade-background-image.html
transitions/cross-fade-border-image.html

[WebKit Review Bot]

Created attachment 133788 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Simon Fraser (smfr)]

Comment on attachment 133776 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:579
> +float CSSLinearGradientValue::getAngle()

Should be const.

We don't normally use 'get' in method names. It should also be clear what the units of the return value are.

Shouldn't we implement the new gradient spec (with the reverse angle behavior) before implementing animation, since all these angles might change?

> Source/WebCore/css/CSSGradientValue.cpp:581
> +    float angle = -1;

-1 seems like a bizarre default. It would be clearer to track the "has not been set yet" state via a separate bool.

> Source/WebCore/css/CSSGradientValue.cpp:585
> +    if (m_firstX.get()) {

Blank line above this one please.

> Source/WebCore/css/CSSGradientValue.h:57
> +    virtual bool canAnimateWith(PassRefPtr<CSSGradientValue> other) const

PassRefPtr<CSSGradientValue> is for transfer of ownership, which is not happening here. Should be a const CSSGradientValue& or const CSSGradientValue*

'canAnimateWith' is a slightly odd name for this. I think we have other code like this which uses the 'blend' terminology.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:41
> +static inline float blendFunc(const AnimationBase*, float from, float to, double progress)
> +{
> +    return narrowPrecisionToFloat(from + (to - from) * progress);
> +}

No need for this. Use WebCore::blend().

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:56
> +    float angle = blendFunc(0, fromAngle, toAngle, amount);

Just use blend() here.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:65
> +    // create result gradient from calculated interpolated angle, to correctly set up end points.

Comments should have sentence case.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:81
> +            blendFunc(0, fromStop.stop, toStop.stop, m_amount),
> +            blendFunc(0, fromStop.red, toStop.red, m_amount),
> +            blendFunc(0, fromStop.green, toStop.green, m_amount),
> +            blendFunc(0, fromStop.blue, toStop.blue, m_amount),
> +            blendFunc(0, fromStop.alpha, toStop.alpha, m_amount)));

Can't you call blend() directly?

So here you're interpolating stops after the length units have been resolved. I think this is wrong; i think you should interpolate CSS Lengths as we do elsewhere (which requires the two gradients to use matching units).

> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:41
> +static inline float blendFunc(const AnimationBase*, float from, float to, double progress)
> +{
> +    return narrowPrecisionToFloat(from + (to - from) * progress);
> +}

Ditto.

> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:85
> +        result->addColorStop(Gradient::ColorStop(blendFunc(0, fromStop.stop, toStop.stop, m_amount),
> +                                                 blendFunc(0, fromStop.red, toStop.red, m_amount),
> +                                                 blendFunc(0, fromStop.green, toStop.green, m_amount),
> +                                                 blendFunc(0, fromStop.blue, toStop.blue, m_amount),
> +                                                 blendFunc(0, fromStop.alpha, toStop.alpha, m_amount)));
> +    }

Ditto.

[Shane Stephens]

Created attachment 133952 [details]
Patch

[Shane Stephens]

Comment on attachment 133776 [details]
Patch

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

>> Source/WebCore/css/CSSGradientValue.cpp:579
>> +float CSSLinearGradientValue::getAngle()
> 
> Should be const.
> 
> We don't normally use 'get' in method names. It should also be clear what the units of the return value are.
> 
> Shouldn't we implement the new gradient spec (with the reverse angle behavior) before implementing animation, since all these angles might change?

fixed method name and constness.

After nearly a year of playing with alternative approaches, I'm very keen to get this checked in. If you like I'll revisit the new gradient spec for both standard and animated gradients once we see some progress on this patch.

>> Source/WebCore/css/CSSGradientValue.cpp:581
>> +    float angle = -1;
> 
> -1 seems like a bizarre default. It would be clearer to track the "has not been set yet" state via a separate bool.

Done.

>> Source/WebCore/css/CSSGradientValue.cpp:585
>> +    if (m_firstX.get()) {
> 
> Blank line above this one please.

Done.

>> Source/WebCore/css/CSSGradientValue.h:57
>> +    virtual bool canAnimateWith(PassRefPtr<CSSGradientValue> other) const
> 
> PassRefPtr<CSSGradientValue> is for transfer of ownership, which is not happening here. Should be a const CSSGradientValue& or const CSSGradientValue*
> 
> 'canAnimateWith' is a slightly odd name for this. I think we have other code like this which uses the 'blend' terminology.

Done. I changed the name to 'canBlendWith' - is that what you'd intended me to do?

>> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:41
>> +}
> 
> No need for this. Use WebCore::blend().

Done.

>> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:56
>> +    float angle = blendFunc(0, fromAngle, toAngle, amount);
> 
> Just use blend() here.

Done.

>> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:65
>> +    // create result gradient from calculated interpolated angle, to correctly set up end points.
> 
> Comments should have sentence case.

Done.

>> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:81
>> +            blendFunc(0, fromStop.alpha, toStop.alpha, m_amount)));
> 
> Can't you call blend() directly?
> 
> So here you're interpolating stops after the length units have been resolved. I think this is wrong; i think you should interpolate CSS Lengths as we do elsewhere (which requires the two gradients to use matching units).

Now calling blend() directly.

As we've discussed over multiple CSSWG meetings, and on www-style, gradient stop resolution (which requires resolution of length units due to the overlapping stop conditions) occurs before interpolation. I tried to mount a defense against doing it in this order on the list and failed; the WG is now as close as it ever will be to consensus that this is the correct approach.

>> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:41
>> +}
> 
> Ditto.

Done.

>> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:85
>> +    }
> 
> Ditto.

Done.

[Build Bot]

Comment on attachment 133952 [details]
Patch

Attachment 133952 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12148168

[WebKit Review Bot]

Comment on attachment 133952 [details]
Patch

Attachment 133952 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12141320

[Shane Stephens]

Created attachment 143439 [details]
Patch

[Build Bot]

Comment on attachment 143439 [details]
Patch

Attachment 143439 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12769081

[WebKit Review Bot]

Comment on attachment 143439 [details]
Patch

Attachment 143439 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12774093

[Shane Stephens]

Created attachment 146483 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146483 [details]
Patch

Attachment 146483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12911523

[Shane Stephens]

Created attachment 147195 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147195 [details]
Patch

Attachment 147195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12943816

New failing tests:
transitions/svg-transitions.html
transitions/cross-fade-background-image.html
transitions/cross-fade-border-image.html

[WebKit Review Bot]

Created attachment 147247 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shane Stephens]

Created attachment 148508 [details]
Patch

[Igor Trindade Oliveira]

Comment on attachment 148508 [details]
Patch

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

I noticed CSSInterpolatedRadialGradientValue and CSSInterpolatedLinearGradientValue have similar attributes/methods, could we create a parent class?

> Source/WebCore/css/CSSImageGeneratorValue.cpp:149
> +        return static_cast<const CSSInterpolatedRadialGradientValue*>(this)->isFixedSize();

we could make this code simpler, something like:
case CSSLinearGradient:
case RadialGradientClass:
case InterpolatedLinearGradientClass:
case InterpolatedRadialGradientClass:
    return static_cast<const CSSGradientValue*>(this)->isFixedSize();

For gradients this method is dumb and it is just implemented in the CSSGradientValue.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:170
> +        return static_cast<const CSSInterpolatedRadialGradientValue*>(this)->fixedSize(renderer);

ditto

> Source/WebCore/dom/Element.cpp:1097
>              if (renderer())

I think we do not need this aesthetic change in this patch. The patch is already huge.

[Shane Stephens]

Thanks for your comments.

I'm going to split this into 3 pieces to make it easier for review. I'll make your proposed changes in the appropriate sub-patches.

[Shane Stephens]

> I noticed CSSInterpolatedRadialGradientValue and CSSInterpolatedLinearGradientValue have similar attributes/methods, could we create a parent class?

It doesn't look like this is going to work. CSSInterpolatedLinearGradientValue needs to inherit from CSSLinearGradientValue; similarly the InterpolatedRadialGradient inherits from CSSRadialGradientValue.

I could use multiple inheritance; but it doesn't seem like multiple inheritance is common practice in WebKit.

[Igor Trindade Oliveira]

Ok this is not a big deal. However I thought differently

CSSGradientValue -> CSSInterpolatedValue -> CSSInterpolatedLinear/GradientValue
 
(In reply to comment #84)
> > I noticed CSSInterpolatedRadialGradientValue and CSSInterpolatedLinearGradientValue have similar attributes/methods, could we create a parent class?
> 
> It doesn't look like this is going to work. CSSInterpolatedLinearGradientValue needs to inherit from CSSLinearGradientValue; similarly the InterpolatedRadialGradient inherits from CSSRadialGradientValue.
> 
> I could use multiple inheritance; but it doesn't seem like multiple inheritance is common practice in WebKit.

[Shane Stephens]

> CSSGradientValue -> CSSInterpolatedValue -> CSSInterpolatedLinear/GradientValue

I just had a play around with this hierarchy to see if I could get it to work.

What it comes down to is that CSSInterpolatedLinearGradient needs to use CSSLinearGradientValue::createGradientEndpoints, which it can't do if it inherits from CSSGradientValue.

I could duplicate some code, or move part of createGradientEndpoints back into CSSGradientValue, but I think that would overall just obfuscate the code. So I'm pretty convinced now that

CSSGradientValue -> CSS[Linear/Radial]GradientValue -> CSInterpolated[Linear/Radial]GradientValue is the right way to go.