Bug 21725 - Should be able to do transitions of background-image, including gradients
: Should be able to do transitions of background-image, including gradients
Status: NEW
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Shane Stephens
: WebExposed
Depends on: 89638
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-17 22:31 PDT by paradox
Modified: 2013-04-20 05:57 PDT (History)
23 users (show)

See Also:


Attachments
Patch (36.36 KB, patch)
2011-05-09 20:38 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.27 MB, application/zip)
2011-05-09 21:09 PDT, WebKit Review Bot
no flags Details
Patch (29.20 KB, patch)
2011-08-09 23:58 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (30.42 KB, patch)
2011-08-29 00:08 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (48.52 KB, patch)
2011-09-06 22:03 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
[preliminary] Use -webkit-cross-fade to implement background-image (and list-style-image, etc.) transitions (13.20 KB, patch)
2011-12-06 18:44 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (60.44 KB, patch)
2012-03-25 03:29 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (59.99 KB, patch)
2012-03-26 04:54 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (9.70 MB, application/zip)
2012-03-26 06:03 PDT, WebKit Review Bot
no flags Details
Patch (60.66 KB, patch)
2012-03-26 18:14 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (65.79 KB, patch)
2012-05-22 20:24 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (66.45 KB, patch)
2012-06-07 22:55 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (66.69 KB, patch)
2012-06-12 17:11 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (999.66 KB, application/zip)
2012-06-13 00:15 PDT, WebKit Review Bot
no flags Details
Patch (67.50 KB, patch)
2012-06-19 22:41 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description paradox 2008-10-17 22:31:05 PDT
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.
Comment 1 Dave Hyatt 2008-10-17 23:19:05 PDT
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.

Comment 2 paradox 2008-10-18 13:40:57 PDT
(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.
Comment 3 Faruk Ates 2009-01-09 15:35:02 PST
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?
Comment 4 Simon Fraser (smfr) 2009-01-09 15:46:24 PST
Please file a separate bug for background-position.
Comment 5 Simon Fraser (smfr) 2009-02-26 12:08:56 PST
Bug 23984 filed on transitions of background-position.
Comment 6 Simon Fraser (smfr) 2009-12-16 11:36:00 PST
*** Bug 26530 has been marked as a duplicate of this bug. ***
Comment 7 Lea Verou 2011-03-21 05:11:55 PDT
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.
Comment 8 Simon Fraser (smfr) 2011-03-21 08:45:43 PDT
Calling it a spec violation is a bit strong. That spec stuff is very new and no-one has implemented it yet.
Comment 9 Lea Verou 2011-03-21 08:53:28 PDT
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.
Comment 10 Shane Stephens 2011-04-05 15:47:55 PDT
I'm interested in taking a look at this, starting with gradients.
Comment 11 Simon Fraser (smfr) 2011-04-05 15:49:31 PDT
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.
Comment 12 Faruk Ates 2011-04-05 15:51:40 PDT
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.
Comment 13 Shane Stephens 2011-04-05 15:52:45 PDT
OK, I'll start with cross-fade then.  I thought gradients would be simpler?
Comment 14 Simon Fraser (smfr) 2011-04-05 15:58:28 PDT
Image cross-fade will force you to set up the right infrastructure.
Comment 15 Dean Jackson 2011-04-05 18:58:38 PDT
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.
Comment 16 Dean Jackson 2011-04-05 19:21:33 PDT
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.
Comment 17 Dean Jackson 2011-04-05 19:29:10 PDT
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.
Comment 18 Faruk Ates 2011-04-05 21:58:52 PDT
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)
Comment 19 Simon Fraser (smfr) 2011-04-05 22:26:44 PDT
Can of worms is officially open.
Comment 20 Tab Atkins 2011-04-05 23:58:37 PDT
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.
Comment 21 Dean Jackson 2011-04-06 11:07:15 PDT
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.
Comment 22 Shane Stephens 2011-04-06 15:08:46 PDT
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?
Comment 23 Tab Atkins 2011-04-06 15:39:44 PDT
(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.
Comment 24 Shane Stephens 2011-05-09 20:38:20 PDT
Created attachment 92913 [details]
Patch
Comment 25 Shane Stephens 2011-05-09 20:44:39 PDT
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 :)
Comment 26 WebKit Review Bot 2011-05-09 21:09:43 PDT
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
Comment 27 WebKit Review Bot 2011-05-09 21:09:48 PDT
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
Comment 28 Simon Fraser (smfr) 2011-05-09 21:29:27 PDT
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.
Comment 29 Shane Stephens 2011-05-10 15:08:58 PDT
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.
Comment 30 Dean Jackson 2011-07-12 18:48:15 PDT
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.
Comment 31 Dean Jackson 2011-07-12 18:49:32 PDT
Obviously I'm not skilled at using the review tool :(

Sorry that the comments have almost no context.
Comment 32 Simon Fraser (smfr) 2011-07-12 20:12:35 PDT
(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).
Comment 33 Shane Stephens 2011-07-12 22:12:04 PDT
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).
Comment 34 Dean Jackson 2011-07-13 05:18:43 PDT
Sounds good!
Comment 35 Tab Atkins 2011-07-13 10:50:04 PDT
(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.
Comment 36 Simon Fraser (smfr) 2011-07-13 11:23:40 PDT
(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.
Comment 37 Tab Atkins 2011-07-13 11:52:12 PDT
(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.
Comment 38 Shane Stephens 2011-08-09 23:58:00 PDT
Created attachment 103447 [details]
Patch
Comment 39 Shane Stephens 2011-08-10 00:13:04 PDT
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.
Comment 40 Tab Atkins 2011-08-10 17:32:20 PDT
(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.
Comment 41 Tab Atkins 2011-08-10 17:32:51 PDT
(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.)
Comment 42 Dean Jackson 2011-08-10 18:07:55 PDT
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.
Comment 43 Tab Atkins 2011-08-10 18:32:28 PDT
(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.
Comment 44 Shane Stephens 2011-08-11 03:53:20 PDT
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.
Comment 45 Tab Atkins 2011-08-11 08:22:17 PDT
(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.
Comment 46 Shane Stephens 2011-08-29 00:08:43 PDT
Created attachment 105465 [details]
Patch
Comment 47 Shane Stephens 2011-08-29 00:09:28 PDT
This latest version includes an implementation of cssText().
Comment 48 WebKit Review Bot 2011-08-29 00:12:20 PDT
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.
Comment 49 WebKit Review Bot 2011-08-29 00:13:45 PDT
Comment on attachment 105465 [details]
Patch

Attachment 105465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9549993
Comment 50 Early Warning System Bot 2011-08-29 00:22:47 PDT
Comment on attachment 105465 [details]
Patch

Attachment 105465 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9566030
Comment 51 Shane Stephens 2011-09-06 22:03:05 PDT
Created attachment 106544 [details]
Patch
Comment 52 WebKit Review Bot 2011-09-06 22:45:58 PDT
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
Comment 53 Dean Jackson 2011-10-12 16:54:54 PDT
ping? shane, do you want a review still? what about the failing tests?
Comment 54 Shane Stephens 2011-10-24 13:31:05 PDT
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.
Comment 55 Tim Horton 2011-12-06 18:44:43 PST
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.
Comment 56 WebKit Review Bot 2011-12-06 19:56:04 PST
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.
Comment 57 Shane Stephens 2011-12-06 20:32:57 PST
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?
Comment 58 Tim Horton 2011-12-06 21:11:51 PST
(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.
Comment 59 Tim Horton 2011-12-08 15:37:02 PST
(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.
Comment 60 Bronislav Klucka 2012-03-20 21:28:05 PDT
(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?
Comment 61 Shane Stephens 2012-03-25 03:29:56 PDT
Created attachment 133674 [details]
Patch
Comment 62 Shane Stephens 2012-03-25 03:31:57 PDT
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.
Comment 63 Shane Stephens 2012-03-26 04:54:30 PDT
Created attachment 133776 [details]
Patch
Comment 64 Build Bot 2012-03-26 05:24:15 PDT
Comment on attachment 133776 [details]
Patch

Attachment 133776 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12133743
Comment 65 Gyuyoung Kim 2012-03-26 05:28:46 PDT
Comment on attachment 133776 [details]
Patch

Attachment 133776 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12127830
Comment 66 WebKit Review Bot 2012-03-26 06:03:24 PDT
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
Comment 67 WebKit Review Bot 2012-03-26 06:03:33 PDT
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
Comment 68 Simon Fraser (smfr) 2012-03-26 11:06:51 PDT
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.
Comment 69 Shane Stephens 2012-03-26 18:14:50 PDT
Created attachment 133952 [details]
Patch
Comment 70 Shane Stephens 2012-03-26 18:24:37 PDT
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.
Comment 71 Build Bot 2012-03-26 18:47:20 PDT
Comment on attachment 133952 [details]
Patch

Attachment 133952 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12148168
Comment 72 WebKit Review Bot 2012-03-26 21:36:58 PDT
Comment on attachment 133952 [details]
Patch

Attachment 133952 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12141320
Comment 73 Shane Stephens 2012-05-22 20:24:43 PDT
Created attachment 143439 [details]
Patch
Comment 74 Build Bot 2012-05-22 20:54:01 PDT
Comment on attachment 143439 [details]
Patch

Attachment 143439 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12769081
Comment 75 WebKit Review Bot 2012-05-22 21:56:27 PDT
Comment on attachment 143439 [details]
Patch

Attachment 143439 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12774093
Comment 76 Shane Stephens 2012-06-07 22:55:01 PDT
Created attachment 146483 [details]
Patch
Comment 77 WebKit Review Bot 2012-06-08 01:03:49 PDT
Comment on attachment 146483 [details]
Patch

Attachment 146483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12911523
Comment 78 Shane Stephens 2012-06-12 17:11:48 PDT
Created attachment 147195 [details]
Patch
Comment 79 WebKit Review Bot 2012-06-13 00:14:52 PDT
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
Comment 80 WebKit Review Bot 2012-06-13 00:15:04 PDT
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
Comment 81 Shane Stephens 2012-06-19 22:41:05 PDT
Created attachment 148508 [details]
Patch
Comment 82 Igor Trindade Oliveira 2012-06-20 18:23:37 PDT
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.
Comment 83 Shane Stephens 2012-06-20 19:03:13 PDT
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.
Comment 84 Shane Stephens 2012-06-21 18:02:40 PDT
> 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.
Comment 85 Igor Trindade Oliveira 2012-06-21 18:14:28 PDT
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.
Comment 86 Shane Stephens 2012-06-25 17:43:09 PDT
> 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.