jfx 8332895: Support interpolation for backgrounds and borders

This PR completes the CSS Transitions story (see #870) by adding interpolation support for backgrounds and borders, making them targetable by transitions.

Background and Border objects are deeply immutable, but not interpolatable. Consider the following Background, which describes the background of a Region:

Background {
    fills = [
        BackgroundFill {
            fill = Color.RED
        }
    ]
}

Since backgrounds are deeply immutable, changing the region's background to another color requires the construction of a new Background, containing a new BackgroundFill, containing the new Color.

Animating the background color using a CSS transition therefore requires the entire Background object graph to be interpolatable in order to generate intermediate backgrounds.

More specifically, the following types will now implement Interpolatable.

Insets
Background
BackgroundFill
BackgroundImage
BackgroundPosition
BackgroundSize
Border
BorderImage
BorderStroke
BorderWidths
CornerRadii
Stop
Paint and all of its subclasses
Margins (internal type)
BorderImageSlices (internal type)

Interpolation of composite objects

As of now, only Color, Point2D, and Point3D are interpolatable. Each of these classes is an aggregate of double values, which are combined using linear interpolation. However, many of the new interpolatable classes comprise of not only double values, but a whole range of other types. This requires us to more precisely define what we mean by "interpolation".

Mirroring the CSS specification, the Interpolatable interface defines several types of component interpolation:

Interpolation type	Description
default	Component types that implement `Interpolatable` are interpolated by calling the `interpolate(Object, double)}` method.
linear	Two components are combined by linear interpolation such that `t = 0` produces the start value, and `t = 1` produces the end value. This interpolation type is usually applicable for numeric components.
discrete	If two components cannot be meaningfully combined, the intermediate component value is equal to the start value for `t < 0.5` and equal to the end value for `t >= 0.5`.
pairwise	Two lists are combined by pairwise interpolation. If the start list has fewer elements than the target list, the missing elements are copied from the target list. If the start list has more elements than the target list, the excess elements are discarded.
(see prose)	Some component types are interpolated in specific ways not covered here. Refer to their respective documentation for more information.

Every component of an interpolatable class specifies its interpolation type with the new @interpolationType javadoc tag. For example, this is the specification of the CornerRadii.topLeftHorizontalRadius component:

    /**
     * The length of the horizontal radii of the top-left corner.
     *
     * @return the length of the horizontal radii of the top-left corner
     * @interpolationType <a href="../../animation/Interpolatable.html#linear">linear</a> if both values are
     *                    absolute or both values are {@link #isTopLeftHorizontalRadiusAsPercentage() percentages},
     *                    <a href="../../animation/Interpolatable.html#discrete">discrete</a> otherwise
     */
    public double getTopLeftHorizontalRadius()

In the generated documentation, this javadoc tag shows up as a new entry in the specification list:

Independent transitions of sub-properties

CSS allows developers to specify independent transitions for sub-properties. Consider the following example:

.button {
    -fx-border-color: red;
    -fx-border-width: 5;

    transition: -fx-border-color 1s linear,
                -fx-border-width 4s ease;
}

.button:hover {
    -fx-border-color: green;
    -fx-border-width: 10;
}

We have two independent transitions (each with a different duration and easing function) that target two sub-properties of the same Border. We can't use normal interpolation between the before-change style and the after-change style of the border, as the Interpolatable interface doesn't allow us to specify separate transitions per sub-property.

Instead, we add a new interface (internal for now) that is implemented by BorderConverter and BackgroundConverter:

public interface SubPropertyConverter<T> {
    /**
     * Converts a map of CSS values to the target type.
     *
     * @param values the constituent values
     * @return the converted object
     */
    T convert(Map<CssMetaData<? extends Styleable, ?>, Object> values);

    /**
     * Converts an object back to a map of its constituent values (deconstruction).
     * The returned map can be passed into {@link #convert(Map)} to reconstruct the object.
     *
     * @param value the object
     * @return a {@code Map} of the constituent values
     */
    Map<CssMetaData<? extends Styleable, ?>, Object> convertBack(T value);
}

This allows us to use BorderConverter and BackgroundConverter to decompose solid objects into their CSS-addressable component parts, animate each of the components, and then reconstruct a new solid object that incorporates the effects of several independent transitions.

More specifically, if the CSS subsystem applies a new value via StyleableProperty.applyStyle(newValue):

If the style converter of newValue implements SubPropertyConverter: a. Deconstruct oldValue and newValue into their components. b. For each component, determine if a transition was specified; if so, start a transition from oldValue.componentN to newValue.componentN with rules as described in "Interpolation of composite objects". c. For each frame, collect the effects of all component transitions, and reconstruct the current value as a solid object.
Otherwise, if newValue implements Interpolatable: Start a regular transition using Interpolatable.interpolate().

Limitations

Implementations usually fall back to discrete interpolation when the start value is an absolute value, and the end value is a percentage (see the example of CornerRadii.topLeftHorizontalRadius above). However, we can often solve these scenarios by first canonicalizing the values before interpolation (i.e. converting percentages to absolute values). This will be a future enhancement.

/reviewers 2 /csr

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
[ ] Change requires CSR request JDK-8333455 to be approved

Issues

JDK-8332895: Support interpolation for backgrounds and borders (Enhancement - P4)
JDK-8226911: Interpolatable's contract should be reexamined (Enhancement - P4)
JDK-8333455: Support interpolation for backgrounds and borders (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
$ git checkout pull/1522

Update a local copy of the PR:
$ git checkout pull/1522
$ git pull https://git.openjdk.org/jfx.git pull/1522/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1522

View PR using the GUI difftool:
$ git pr show -t 1522

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1522.diff

Webrev

Link to Webrev Comment

Jul 30 '24 16:07 mstr2

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Jul 30 '24 16:07 bridgekeeper[bot]

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332895: Support interpolation for backgrounds and borders
8226911: Interpolatable's contract should be reexamined

Reviewed-by: angorya, jhendrikx, nlisker, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

5428f267887ea4098f6c2a87335de7ed9bf5c656: 8340982: [win] Dead key followed by Space generates two characters instead of one
7870a226a21826e3979e314c0218d351b3cfb82f: 8297072: Provide gradle option to test a previously built SDK
ecab6b6b0eab288eff1e13173a79a2fa4f4aca80: 8340980: Cannot build on Windows ARM
0dd0c794c3b08f816e7618026d5c90deaf952046: 8340954: Add SECURITY.md file

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Jul 30 '24 16:07 openjdk[bot]

@mstr2 The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Jul 30 '24 16:07 openjdk[bot]

@mstr2 an approved CSR request is already required for this pull request.

Jul 30 '24 16:07 openjdk[bot]

Webrevs

Jul 30 '24 16:07 mlbridge[bot]

I note that PR #1471 was originally proposed to address this. Can you highlight the differences? Are any of the comments added to that PR still relevant?

I also note that JDK-8226911 was added as an issue to the original PR? Should it also be added here?

I'd like a review by at least two with a "Reviewer" role in addition to the CSR.

/reviewers 2 reviewers

Aug 01 '24 22:08 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

Aug 01 '24 22:08 openjdk[bot]

I note that PR #1471 was originally proposed to address this. Can you highlight the differences? Are any of the comments added to that PR still relevant?

The CSS specification allows specifying separate transitions for sub-properties (see "Independent transitions of sub-properties" in this PR). The previous implementation didn't account for that, while this implementation does. The comments are still applicable where they concern the Interpolatable implementations.

I also note that JDK-8226911 was added as an issue to the original PR? Should it also be added here?

Yes, I will add it.

Aug 02 '24 00:08 mstr2

/issue add JDK-8226911

Aug 02 '24 00:08 mstr2

@mstr2 Adding additional issue to issue list: 8226911: Interpolatable's contract should be reexamined.

Aug 02 '24 00:08 openjdk[bot]

This is still a large PR, and I do want to help reviewing it, I will do my best to look it over further this week.

Aug 04 '24 22:08 hjohn

The code does not build in Eclipse. Could you please replace modules/graphics/.classpath with this:

<?xml version="1.0" encoding="UTF-8"?>
<classpath>
	<classpathentry kind="src" path="src/main/java"/>
	<classpathentry kind="src" path="build/gensrc/jsl-prism"/>
	<classpathentry kind="src" path="build/gensrc/jsl-decora"/>
	<classpathentry kind="src" path="build/hlsl/Decora"/>
	<classpathentry kind="src" path="build/hlsl/Prism"/>
	<classpathentry kind="src" output="testbin" path="src/shims/java">
		<attributes>
			<attribute name="test" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" output="testbin" path="src/test/java">
		<attributes>
			<attribute name="test" value="true"/>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" path="src/main/resources">
		<attributes>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" output="testbin" path="src/test/resources">
		<attributes>
			<attribute name="test" value="true"/>
			<attribute name="optional" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry combineaccessrules="false" kind="src" path="/base">
		<attributes>
			<attribute name="module" value="true"/>
			<attribute name="add-exports" value="javafx.base/com.sun.javafx.property=javafx.graphics:javafx.base/test.javafx.collections=javafx.graphics:javafx.base/test.util.memory=javafx.graphics:javafx.base/test.util=javafx.graphics"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5">
		<attributes>
			<attribute name="test" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
		<attributes>
			<attribute name="module" value="true"/>
			<attribute name="add-exports" value="java.base/sun.security.util=javafx.graphics"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="output" path="bin"/>
</classpath>

(that simply adds a missing dependency :javafx.base/test.util=javafx.graphics )

And also merge the latest master in please?

Aug 06 '24 19:08 andy-goryachev-oracle

This transition does not interpolate linearly. Should it?

.button {
}

.button:hover {
  -fx-font-size:200%;
  transition: 
    -fx-font-size 2s linear;
}

Aug 07 '24 16:08 andy-goryachev-oracle

This transition does not interpolate linearly. Should it?
.button {
}

.button:hover {
  -fx-font-size:200%;
  transition: 
    -fx-font-size 2s linear;
}

Font is not Interpolatable, and its style converter does not support reconstruction. Thus it falls back to discrete interpolation. I'll file a new ticket to track this enhancement: JDK-8338002

Aug 07 '24 16:08 mstr2

I could not find in cssref.html any reference to which properties are interpolatable, and which components of interpolatable and which support reconstruction. I think we need to update the spec with this information.

For example, the following does not interpolate, is it because I am doing something wrong or the border radius is not interpolatable?

.button {
 -fx-border-radius: 100;
}

.button:hover {
 -fx-border-radius: 1;
  transition: 
   -fx-border-radius 2s linear;
}

Aug 07 '24 17:08 andy-goryachev-oracle

Somewhat tangential question: perhaps this enhancement needs a JEP (or a JEP-formatted doc) for future reference. The github infrastructure may one day disappear but I think these kinds of documents should be preserved.

Even more tangential question is where to put these docs? For example, I think a good place might be in /doc-files somewhere, makes the review process easier. We've been creating these docs in private repos which I think is not a good place for the final product (i.e. when the feature is integrated).

What do you think? (I will also post this question in the mailing list).

Aug 07 '24 17:08 andy-goryachev-oracle

I could not find in cssref.html any reference to which properties are interpolatable, and which components of interpolatable and which support reconstruction. I think we need to update the spec with this information.

cssref.html says: JavaFX supports implicit transitions for properties that are styled by CSS. When a property value is changed by CSS, it transitions to its new value over a period of time. Implicit transitions are supported for all primitive types, as well as for types that implement javafx.animation.Interpolatable.

So all styleable primitive properties are transitionable, as well as all styleable Interpolatable properties. Composite properties like Background and Border need a bit of support from their style converter, but I would almost consider this an implementation detail from the perspective of the user.

For example, the following does not interpolate, is it because I am doing something wrong or the border radius is not interpolatable?
.button {
 -fx-border-radius: 100;
}

.button:hover {
 -fx-border-radius: 1;
  transition: 
   -fx-border-radius 2s linear;
}

You probably don't see anything because Modena's button doesn't have a visible border. Try specifying some color to make it visible:

.button {
    -fx-border-radius: 10,
    -fx-border-color: red;
}

Aug 07 '24 18:08 mstr2

Somewhat tangential question: perhaps this enhancement needs a JEP (or a JEP-formatted doc) for future reference. The github infrastructure may one day disappear but I think these kinds of documents should be preserved.

Even more tangential question is where to put these docs? For example, I think a good place might be in /doc-files somewhere, makes the review process easier. We've been creating these docs in private repos which I think is not a good place for the final product (i.e. when the feature is integrated).

What do you think? (I will also post this question in the mailing list).

Isn't this the purpose of the CSR documents?

Aug 07 '24 18:08 mstr2

Isn't this the purpose of the CSR documents?

These two are related but not the same, per https://wiki.openjdk.org/display/csr/CSR+FAQs :

Q: What is the relationship between a CSR and a JEP? A: A JEP (JDK Enhancement-Proposal) initially describes a project to update some aspect of the JDK. The exact details of the updates are usually not yet known when the JEP begins. Before a JEP is advanced to the Candidate state, it is strongly recommended the CSRs of a JEP go through at least the first phase of CSR review to Provisional state. Before a JEP is advanced to the Proposed to Target state, CSRs associated with the JEP must have advanced through to at least the CSR Provisional state. For Proposed to Target, the CSRs covering the work of a JEP may also go fully through the CSR process to the Approved state rather than the Provisional state. As the JEP matures, the updates to the JDK associated with the JEP are pushed under one or more changesets. Each changeset that involves a specification change (or sufficiently large compatibility impact) would also require CSR review, just as done for other changes to the JDK.

Aug 07 '24 18:08 andy-goryachev-oracle

Regarding cssref.html:

I think it might make sense to add a section after "Transitions" talking about component transitions, maybe with examples.
I think we should to mention interpolation behavior next to each property name somehow. Either mark the properties that do support it, or only those that do not support. It is already documented in javadoc, but cssref.html is a normative document for CSS.

what do you think?

Aug 07 '24 20:08 andy-goryachev-oracle

Just wanted to mention that so far my testing using the Monkey Tester look good - the behavior is as expected (except for interpolating colors which does not use the right algorithm, but that's a different issue).

Aug 07 '24 22:08 andy-goryachev-oracle

Regarding cssref.html:

I think it might make sense to add a section after "Transitions" talking about component transitions, maybe with examples.

I think we should to mention interpolation behavior next to each property name somehow. Either mark the properties that do support it, or only those that do not support. It is already documented in javadoc, but cssref.html is a normative document for CSS.

what do you think?

I'm not sure what to say here, and I question whether introducing the concept of component-wise transitions to users is useful at all. This is more of an implementation detail, as from the perspective of CSS (where transitions are specified), there is no such thing: the reason why you can target -fx-border-color with a transition is because the property type is Paint, which implements Interpolatable. The fact that Interpolatable types support transitions is already mentioned in cssref.html; elaborating on terminology that has no further use in CSS might be even more confusing for users.

There shouldn't be any need to either document support or non-support with regards to transitions, because the current language is already clear that if the property is not Interpolatable, there will be no transition.

Aug 08 '24 01:08 mstr2

the current language is already clear that if the property is not Interpolatable, there will be no transition.

I am talking about cssref.html and not the code. One should be able to see whether a property is interpolatable from reading the cssref.html spec without having to dig into the code.

The word "interpolatable" occurs in cssref.html in exactly one place. What I think the reader should be able to do is to find out whether a property is interpolatable from reading the section about said property in cssref.html.

In other words, there should be info about interpolation in the property table: Screenshot 2024-08-08 at 08 29 32

Since probably most of the properties are "interpolatable" we can mention that the particular property does not support interpolation instead.

Another example: until JDK-8338002 is implemented, -fx-font* interpolates discretely, which should be reflected in the css spec.

Or am I missing something?

Aug 08 '24 15:08 andy-goryachev-oracle

Since probably most of the properties are "interpolatable" we can mention that the particular property does not support interpolation instead.

Another example: until JDK-8338002 is implemented, -fx-font* interpolates discretely, which should be reflected in the css spec.

Or am I missing something?

Maybe we should do that as a separate ticket (JDK-8338121). Changing the documentation of properties that have nothing to do with backgrounds and borders is probably a bit out of scope for this PR.

Aug 09 '24 02:08 mstr2

Maybe we should do that as a separate ticket. Changing the documentation of properties that have nothing to do with backgrounds and borders is probably a bit out of scope for this PR.

I am afraid I'd disagree here: I would like to see the CSS reference reflect the new changes, to be in sync with the javadoc. I think this is still a part of the same changeset.

Aug 09 '24 14:08 andy-goryachev-oracle

Maybe we should do that as a separate ticket. Changing the documentation of properties that have nothing to do with backgrounds and borders is probably a bit out of scope for this PR.

I am afraid I'd disagree here: I would like to see the CSS reference reflect the new changes, to be in sync with the javadoc. I think this is still a part of the same changeset.

I'm a bit behind this week due to other commitments, but wanted to chime in briefly. The main question in my mind is whether (and how) this particular aspect of a property should be documented in the cssref. We don't duplicate all aspects of each styleable property in cssref. So, is the fact that a property is interpolatable important enough to CSS operation that it should be duplicated in cssref or not? I haven't had time to form an opinion, but that's the question that should be answered first.

Aug 09 '24 15:08 kevinrushforth

CSS properties don't always correspond to JavaFX properties (for example, -fx-background-color has no corresponding JavaFX property, it is combined with other CSS properties and mapped to backgroundProperty()). It might be a good idea to add a column to the CSS property tables, indicating the animation type of each CSS property. It would look like this:

animationtype

However, this is a large documentation change in and of itself, and I strongly suggest to do this as part of JDK-8338121 to keep this PR manageable and focused.

Aug 10 '24 17:08 mstr2

I strongly suggest to do this as part of JDK-8338121

+1. I am fine with splitting the work in two parts. My only suggestion is to valign the table cells to top instead of center.

Aug 12 '24 14:08 andy-goryachev-oracle

Mirroring the CSS specification, the Interpolatable interface defines several types of component interpolation

Can you give a link to these CSS specifications?

Aug 17 '24 15:08 nlisker

Mirroring the CSS specification, the Interpolatable interface defines several types of component interpolation

Can you give a link to these CSS specifications?

I shouldn't say "mirroring", it's more "inspired by".

https://www.w3.org/TR/web-animations/#animating-properties

Aug 17 '24 17:08 mstr2

jfx jfx copied to clipboard

8332895: Support interpolation for backgrounds and borders

Interpolation of composite objects

Independent transitions of sub-properties

Limitations

Progress

Issues

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard