8332895: Support interpolation for backgrounds and borders #1522

mstr2 · 2024-07-30T16:03:46Z

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

Change must not contain extraneous whitespace
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)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
John Hendrikx (@hjohn - Reviewer)
Nir Lisker (@nlisker - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

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

bridgekeeper · 2024-07-30T16:04:35Z

👋 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.

openjdk · 2024-07-30T16:05:03Z

@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:

5428f26: 8340982: [win] Dead key followed by Space generates two characters instead of one
7870a22: 8297072: Provide gradle option to test a previously built SDK
ecab6b6: 8340980: Cannot build on Windows ARM
0dd0c79: 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.

openjdk · 2024-07-30T16:05:47Z

@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).

openjdk · 2024-07-30T16:06:42Z

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

mlbridge · 2024-07-30T16:09:14Z

Webrevs

kevinrushforth · 2024-08-01T22:10:31Z

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

openjdk · 2024-08-01T22:12:07Z

@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).

nlisker

Approving from the perspective of the API with the remark that there is a theoretical API break for TransitionEvent.

mstr2 · 2024-09-14T16:54:47Z

Approving from the perspective of the API with the remark that there is a theoretical API break for TransitionEvent.

Thanks for the review! I added the changed TransitionEvent API as a compatibility risk in the CSR.

andy-goryachev-oracle

latest changes are an improvement, thank you

# Conflicts: # modules/javafx.graphics/src/test/java/test/javafx/geometry/InsetsTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundFillTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundImageTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundPositionTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundSizeTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderStrokeTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderWidthsTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ColorTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ImagePatternTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/LinearGradientTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopListTest.java # modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopTest.java

kevinrushforth

This looks really good and is almost ready to go in. Since the implementation has been heavily reviewed already, I'll limit my review to the API and do some light testing.

I left a few comments on the API and docs inline. In addition, should ImagePattern::getImage specify an interpolation type of "discrete"? The docs for that method weren't updated.

As soon as these issues are addressed, I'll re-review and also Review the CSR.

kevinrushforth · 2024-09-27T17:53:49Z

modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundFill.java

     * @return the Paint to use for filling the background of the {@link Region}
+     * @interpolationType see {@link BackgroundFill}


There isn't any mention of how interpolation is done in the class docs of this class. Did you mean to add something to the class docs, or should it link to Paint instead?

kevinrushforth · 2024-09-27T18:50:53Z

modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java

+ *         </tr>
+ *         <tr><td style="vertical-align: top"><a id="linear" style="white-space: nowrap">linear</a></td>
+ *             <td>Two components are combined by linear interpolation such that {@code t = 0} produces
+ *                 the start value, and {@code t = 1} produces the end value. This interpolation type


Minor: I might add something like: and {@code 0 < t < 1} produces {@code (1 - t) * start + t * end}, to clearly define what linear means. This would be OK to add in a follow-up.

kevinrushforth · 2024-09-27T18:57:40Z

modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java

     * @param elapsedTime the time that has elapsed since the transition has entered its active period
     * @throws NullPointerException if {@code eventType}, {@code property} or {@code elapsedTime} is {@code null}
     */
    public TransitionEvent(EventType<? extends Event> eventType,
                           StyleableProperty<?> property,
+                           String propertyName,


It might be best to leave the original constructor, deprecate it for removal, and set the default value of name to the empty string. We could then remove it in JavaFX 25.

Either way, this new constructor will need an @since 24 tag.

kevinrushforth

Thanks for the updates. Looks good with a couple more comments on the deprecated-for-removal constructor.

kevinrushforth · 2024-09-27T22:10:54Z

modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java

     * @param elapsedTime the time that has elapsed since the transition has entered its active period
     * @throws NullPointerException if {@code eventType}, {@code property} or {@code elapsedTime} is {@code null}
     */
+    @Deprecated(forRemoval = true)


Can you add since = 24, before the forRemoval?

Also please add an @deprecated javadoc tag referring to the new constructor.

kevinrushforth

Thanks, it looks good now. I have also Reviewed the CSR. You can Finalize it when ready.

nlisker

Re-approving the API.

andy-goryachev-oracle

re-approving

mstr2 · 2024-10-01T19:30:16Z

/integrate

openjdk · 2024-10-01T19:31:23Z

Going to push as commit 01e9e7e.
Since your change was applied there have been 4 commits pushed to the master branch:

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

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-01T19:31:31Z

@mstr2 Pushed as commit 01e9e7e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mstr2 added 19 commits June 2, 2024 20:26

Add interpolation support for backgrounds and borders

25bcb1d

add since tags

e795fd0

clean up imports

bb84c57

Merge branch 'master' into feature/interpolatable

4f363a4

added specification and tests

e7e5fac

added more tests

3221441

revert change

5f63c28

revert change

47e55bf

add exports

1cebcc1

add specification

831cf6b

change documentation

1efce84

add documentation to Point2D/3D

12a73d5

fix line separators

08ed751

ComponentTransitionable implementation

6dbefb8

fix line separators

bbfb025

Merge branch 'master' into feature/interpolatable

ab8480a

add tests

1cc8db8

documentation

7650fea

fix line separators

94d6a90

openjdk bot added csr Need approved CSR to integrate pull request rfr Ready for review labels Jul 30, 2024

fixed a bug

675bc7f

kevinrushforth self-requested a review August 1, 2024 22:11

nlisker approved these changes Sep 14, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Sep 16, 2024

View reviewed changes

javadoc change

1c842cc

hjohn approved these changes Sep 17, 2024

View reviewed changes

mstr2 added 2 commits September 25, 2024 04:02

fix merge conflicts

3027caa

andy-goryachev-oracle approved these changes Sep 25, 2024

View reviewed changes

kevinrushforth reviewed Sep 27, 2024

View reviewed changes

mstr2 added 2 commits September 28, 2024 00:05

documentation changes

bb0cc60

documentation changes

e3792cc

kevinrushforth reviewed Sep 27, 2024

View reviewed changes

mstr2 added 2 commits September 28, 2024 00:16

removed unnecessary imports

7d086cf

added @deprecated javadoc tag

332a5c1

kevinrushforth approved these changes Sep 28, 2024

View reviewed changes

kevinrushforth requested a review from andy-goryachev-oracle September 28, 2024 13:09

mstr2 requested review from nlisker and hjohn September 29, 2024 20:34

nlisker approved these changes Sep 30, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Sep 30, 2024

View reviewed changes

hjohn approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Oct 1, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 1, 2024

openjdk bot closed this Oct 1, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332895: Support interpolation for backgrounds and borders #1522

8332895: Support interpolation for backgrounds and borders #1522

mstr2 commented Jul 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024 •

edited

Loading

openjdk bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024

mlbridge bot commented Jul 30, 2024 •

edited

Loading

kevinrushforth commented Aug 1, 2024

openjdk bot commented Aug 1, 2024

nlisker left a comment

mstr2 commented Sep 14, 2024

andy-goryachev-oracle left a comment

kevinrushforth left a comment •

edited

Loading

kevinrushforth Sep 27, 2024

kevinrushforth Sep 27, 2024

kevinrushforth Sep 27, 2024

kevinrushforth left a comment

kevinrushforth Sep 27, 2024

kevinrushforth left a comment

nlisker left a comment

andy-goryachev-oracle left a comment

mstr2 commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

		* @return the Paint to use for filling the background of the {@link Region}
		* @interpolationType see {@link BackgroundFill}

8332895: Support interpolation for backgrounds and borders #1522

8332895: Support interpolation for backgrounds and borders #1522

Conversation

mstr2 commented Jul 30, 2024 • edited by openjdk bot Loading

Interpolation of composite objects

Independent transitions of sub-properties

Limitations

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024 • edited Loading

openjdk bot commented Jul 30, 2024

openjdk bot commented Jul 30, 2024

mlbridge bot commented Jul 30, 2024 • edited Loading

Webrevs

kevinrushforth commented Aug 1, 2024

openjdk bot commented Aug 1, 2024

nlisker left a comment

Choose a reason for hiding this comment

mstr2 commented Sep 14, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment • edited Loading

Choose a reason for hiding this comment

kevinrushforth Sep 27, 2024

Choose a reason for hiding this comment

kevinrushforth Sep 27, 2024

Choose a reason for hiding this comment

kevinrushforth Sep 27, 2024

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Sep 27, 2024

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

nlisker left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

mstr2 commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

mstr2 commented Jul 30, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 30, 2024 •

edited

Loading

mlbridge bot commented Jul 30, 2024 •

edited

Loading

kevinrushforth left a comment •

edited

Loading