Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:15:37 GMT, drmarmac wrote: > * 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. CSS transitions work with styleable properties that are either primitives or `Interpolatable` objects. So if you apply a transition to the sub-property `-fx-background-color`, what really happens is that the CSS subsystem creates a new `Background` instance and applies it to the `Region.background` property. However, since `Background` is not interpolatable, you won't see an animation. This will start to work once all relevant classes are interpolatable, which will come with a subsequent PR. I'm not sure if we really need a warning. The unexpected non-animation is really only due to a few missing interpolatable classes, which will be fixed soon. > * Same with the current limitation of not being able to mix shorthand and > longhand notations, do we need a warning if it is attempted? This is out of scope for this PR, since the problem is not unique to the CSS `transition` property. We should improve the CSS parser to handle shorthand/longhand notations uniformly. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2129318991
Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:02:44 GMT, drmarmac wrote:
>> 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
>
> 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)
Good catch! I've also updated 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?
I've replaced `"all"` with a named constant in all relevant places.
> modules/javafx.graphics/src/test/java/test/javafx/scene/Node_transition_Test.java
> line 162:
>
>> 160: List 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.
I've changed the test to use `-fx-fill` instead.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307416
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307906
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613308024
Re: RFR: 8311895: CSS Transitions [v16]
On Thu, 2 May 2024 08:40:28 GMT, Michael Strauß 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 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
Re: RFR: 8311895: CSS Transitions [v16]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif";
> width="200"/>
>
> ### 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
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=15
Stats: 4673 lines in 43 files changed: 4630 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
