RFR: 8311895: CSS Transitions [v16]

[drmarmac]

On Thu, 2 May 2024 08:40:28 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 53 commits:
> 
>  - 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
>  - Changes per review
>  - Merge branch 'master' into feature/css-transitions
>  - Merge branch 'master' into feature/css-transitions
>  - Add TransitionMediator
>  - ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9

I did some testing of this PR and started looking into the implementation (see code comments).
- Fade in / fade out with delay and various easing functions via the opacity property works as expected, , scaleX/scaleY also look good
- Attempting to do background-color transitions doesn't work, I presume because it's not yet `Interpolatable` in this PR. Do we need to emit a warning in such a case? Right now the `transition` CSS code just seems to be ignored.
- Same with the current limitation of not being able to mix shorthand and longhand notations, do we need a warning if it is attempted?
- Generally this looks like a high quality PR, filling a long-standing gap. Currently available alternative solutions (e.g. using code-driven animations) are quite cumbersome and not easy to get right.

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

> 289:          * All rise points are within the open interval (0..1).
> 290:          */
> 291:         BOTH,

Looks like the docs for BOTH and NONE are swapped? (this would also affect the CSR)

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9081:

> 9079:                 TransitionDefinition transition = get(i);
> 9080: 
> 9081:                 boolean selected = "all".equals(transition.propertyName())

Is "all" the same as `TransitionDefinitionCssMetaData.PROPERTY_ALL`, so the magic string can be avoided?

modules/javafx.graphics/src/test/java/test/javafx/scene/Node_transition_Test.java line 162:

> 160:         List<TransitionDefinition> transitions = NodeShim.getTransitionDefinitions(node);
> 161:         assertEquals(3, transitions.size());
> 162:         assertTransitionEquals("-fx-background-color", Duration.seconds(1), Duration.seconds(0.5), CSS_EASE, transitions.get(0));

`node` is a `Rectangle`, which doesn't have a `background`property. Doesn't hurt this specific test, but maybe it's better to use an existing property.

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

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2076453636
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613220507
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613227193
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613233729