Re: RFR: 8311895: CSS Transitions [v19]
On Sat, 25 May 2024 21:39:24 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 incrementally with one additional
> commit since the last revision:
>
> added documentation
modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
749:
> 747: The property value is set programmatically
> 748: The property is bound
> 749: The node becomes invisible
I would mention that this relates to the `visible` property and not to the
`opacity` one (the node is invisible if opacity is 0).
Other than that, looks good.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614945779
Re: RFR: 8311895: CSS Transitions [v18]
On Sat, 25 May 2024 21:17:06 GMT, Nir Lisker wrote: > This is what I would expect, so looks good. Where is this mentioned to the > user? Good question. Since we don't have any suitable API elements for javadocs, I've added some documentation to the CSS reference. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131488168
Re: RFR: 8311895: CSS Transitions [v19]
> 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 incrementally with one additional
commit since the last revision:
added documentation
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/d3184e6c..4cb6c876
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=18
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=17-18
Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 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
Re: RFR: 8311895: CSS Transitions [v18]
On Sat, 25 May 2024 20:40:39 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 incrementally with one additional
> commit since the last revision:
>
> address review comments
This is what I would expect, so looks good. Where is this mentioned to the user?
-
PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131460092
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:39:12 GMT, Nir Lisker wrote: > 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. Any of the following actions will cancel a running transition: 1. Setting the property value 2. Binding the property 3. Making the node invisible (i.e. `isTreeVisible()` returns false) 4. Removing the node from the scene graph In effect, transitions are only active when "left alone", any programmatic interaction will cancel them. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131459204
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 21:04:24 GMT, Michael Strauß wrote: >> 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 >>> javafx.animation.Interpolatable. >>> 690: Transitions can be defined for any node in the JavaFX scene >>> graph with the following properties: >> >> 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. > > I understand that you're saying that `property`, `duration`, > `timing-function`, and `delay` are all sub-properties of `transition`. > > However, from a CSS perspective, `transition-property`, > `transition-duration`, `transition-timing-function` and `transition-delay` > are properties of `Node`, in the same way as `-fx-background-color`, > `-fx-background-insets`, etc. are properties of `Node`. > > `transition` is a short-hand property that combines all four properties into > one (we don't have a short-hand property for backgrounds yet). I think that > both mental models are basically correct (four properties of node, vs. four > sub-properties of transition). I understand. I find it a bit confusing, but OK to leave as is. - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614892590
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:35:56 GMT, Nir Lisker 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 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 > > 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 >> javafx.animation.Interpolatable. >> 690: Transitions can be defined for any node in the JavaFX scene >> graph with the following properties: > > 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. I understand that you're saying that `property`, `duration`, `timing-function`, and `delay` are all sub-properties of `transition`. However, from a CSS perspective, `transition-property`, `transition-duration`, `transition-timing-function` and `transition-delay` are properties of `Node`, in the same way as `-fx-background-color`, `-fx-background-insets`, etc. are properties of `Node`. `transition` is a short-hand property that combines all four properties into one (we don't have a short-hand property for backgrounds yet). I think that both mental models are basically correct (four properties of node, vs. four sub-properties of transition). - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614886767
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:28:56 GMT, Nir Lisker 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 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 > > modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line > 688: > >> 686: Transitions >> 687: JavaFX supports implicit transitions 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? I've changed the wording a bit. > 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. The W3C specification for the [transition-delay](https://www.w3.org/TR/css-transitions-1/#transition-delay-property) property mandates it, and this PR is a complete implementation of the specification. You're right that `Animation` doesn't support negative delays, but at least for implicit transitions, negative delays have some utility. Maye we can revisit the choice to disallow negative delays for `Animation` later. - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614870557 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614870213
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 20:37:44 GMT, Michael Strauß wrote:
>> 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.
>
> I've included the images that are also used in the CSS reference
> documentation. Now there are two copies of these images in two different
> `doc-files` folders, but I guess that's okay.
I think it's fine. Another option is to link to the part of the reference where
they are.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614872766
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 17:40:01 GMT, Nir Lisker 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 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
>
> 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.
Changed the parameter name to `intervalCount`.
I agree that the uppercase naming scheme in `Interpolator` is a bit
unfortunate, but at this point I think consistency is more important. We're
unlikely to add more built-in interpolators in any case.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614865200
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 10:40:05 GMT, Nir Lisker 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 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
>
> 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.
I've included the images that are also used in the CSS reference documentation.
Now there are two copies of these images in two different `doc-files` folders,
but I guess that's okay.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614863775
Re: RFR: 8311895: CSS Transitions [v18]
> 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 incrementally with one additional
commit since the last revision:
address review comments
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/a43dee30..d3184e6c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=17
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=16-17
Stats: 28 lines in 7 files changed: 8 ins; 0 del; 20 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
Re: RFR: 8311895: CSS Transitions [v2]
On Mon, 31 Jul 2023 18:10:46 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java
>> line 319:
>>
>>> 317: * The output time value is determined by the {@link StepPosition}.
>>> 318: *
>>> 319: * @param intervals the number of intervals in the step
>>> interpolator
>>
>> minor: When I see a plural like `intervals` (or `employees`) I think of a
>> list of objects. Perhaps `intervalCount` would be better?
>
> Doesn't sound better to me, but I'll defer to what most people feel is best.
I somewhat agree with John. I would probably just use `numberOfIntervals`, but
`intervalCount` of `numIntervals` is also fine.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614560326
Re: RFR: 8311895: CSS Transitions [v17]
On Fri, 24 May 2024 11:18:35 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 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: Transitions
> 687: JavaFX supports implicit transitions 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
> javafx.animation.Interpolatable.
> 690: Transitions can be defined for any node in the JavaFX scene graph
> with the following properties:
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_
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Fri, 24 May 2024 15:04:08 GMT, Nir Lisker wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move getStyleClassNames to location it was introduced to reduce diff > > The code looks good. I didn't test it, but I'm fine with integrating. > Also some clarification on the contributing rules: "all Reviewers who have > requested the chance to review have done so" -- does the indication at the > top right of the PR count towards this or should it be a comment? :) In the > first case, @nlisker and @arapte, please indicate if you wish to review this > still. If someone wants you to wait for them, they should make it clear by adding a comment. Also if someone has given substantive feedback, but hasn't (re)approved, it's good to give them a change to review. @arapte can add a comment if he wants to review, otherwise go ahead and integrate on Monday. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2131275284
