RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Fri, 24 May 2024 11:18:35 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Implementation of [CSS Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the `Interpolatable` interface. For example, targeting `-fx-background-color` only works if all background-related objects are interpolatable: `Color`, `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the `transition` property. However, due to limitations of JavaFX CSS, mixing both notations doesn't work:
>> 
>> .button {
>>     transition: -fx-background-color 1s;
>>     transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits:
> 
>  - Merge branch 'refs/heads/master' into feature/css-transitions
>  - extract magic string to named constant
>  - use existing property in test
>  - fixed documentation
>  - Merge branch 'master' into feature/css-transitions
>  - update 'since' tags
>  - Fix javadoc error
>  - Change javadoc comment
>  - Merge branch 'master' into feature/css-transitions
>  - Discard redundant transitions in StyleableProperty impls
>  - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30

I did a quick review of some of the code, mostly the API. I still don't know what happens when a value is programmatically set while a css transition is in progress. What I understood is that binding the property will not allow the transition to start/continue, but didn't see where setting a value was mentioned.

Otherwise looks good. I don't intend to review this beyond my current comments, so you can integrate once resolved.

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 688:

> 686:     <h3><a id="introtransitions">Transitions</a></h3>
> 687:     <p>JavaFX supports <em>implicit transitions</em> for properties that are styled by CSS. When a property value is
> 688:         changed, it smoothly transitions to the new value over a period of time. Implicit transitions are supported

Maybe not so smoothly when using a step interpolator?

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 690:

> 688:         changed, it smoothly transitions to the new value over a period of time. Implicit transitions are supported
> 689:         for all primitive types, as well as for types that implement <code>javafx.animation.Interpolatable</code>.</p>
> 690:     <p>Transitions can be defined for any node in the JavaFX scene graph with the following properties:</p>

The way this is phrased makes it sound like the node has "the following properties", not the transition. Maybe move that part:
"Transitions with the following properties can be defined for any node in the JavaFX scene graph", or just add a comma.

modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java line 40:

> 38:  * @param duration duration of the transition
> 39:  * @param delay delay after which the transition is started; if negative, the transition starts
> 40:  *              immediately, but will appear to have begun at an earlier point in time

Why accept a negative delay? An [`Animation`](https://openjfx.io/javadoc/22/javafx.graphics/javafx/animation/Animation.html#getDelay()) doesn't accept it.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 277:

> 275:      * @since 23
> 276:      */
> 277:     public enum StepPosition {

I think it would be helpful to include (or link to) images that show what the steps for each option looks like. The verbal description is a bit technical.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 328:

> 326:      * @since 23
> 327:      */
> 328:     public static Interpolator STEPS(int intervals, StepPosition position) {

Static method names shouldn't be named like constants, although `Interpolator` does this for other methods already. Not sure if this trend should continue.

-------------

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2078912077
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614833351
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614836286
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614815486
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614554508
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614789417