jfx 8311895: CSS Transitions

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.

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
[ ] Change requires CSR request JDK-8313383 to be approved

Issues

JDK-8311895: CSS Transitions (Enhancement - P4)
JDK-8313383: CSS Transitions (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
$ git checkout pull/870

Update a local copy of the PR:
$ git checkout pull/870
$ git pull https://git.openjdk.org/jfx.git pull/870/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 870

View PR using the GUI difftool:
$ git pr show -t 870

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/870.diff

Webrev

Link to Webrev Comment

Aug 16 '22 03:08 mstr2

:wave: 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.

Aug 16 '22 03:08 bridgekeeper[bot]

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/css-transitions
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Feb 17 '23 18:02 openjdk[bot]

@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Apr 14 '23 20:04 bridgekeeper[bot]

Hi @shaoerkuai, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user shaoerkuai for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

[ ] I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Jun 04 '23 15:06 shaoerkuai

@mstr2 This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Jul 13 '23 19:07 bridgekeeper[bot]

/open

Jul 13 '23 19:07 mstr2

@mstr2 This pull request is now open

Jul 13 '23 19:07 openjdk[bot]

If this is moved forward, it will need a CSR.

/csr

Jul 14 '23 17:07 kevinrushforth

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8311895 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Jul 14 '23 17:07 openjdk[bot]

Webrevs

Jul 26 '23 03:07 mlbridge[bot]

Wow, replacing some Java code with CSS can make the Java code more focused on business logic, making it much cleaner.

Jul 26 '23 14:07 leewyatt

/reviewers 3

Jul 29 '23 14:07 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

Jul 29 '23 14:07 openjdk[bot]

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Sep 06 '23 18:09 bridgekeeper[bot]

Let’s keep this open.

Sep 06 '23 19:09 mstr2

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Oct 04 '23 23:10 bridgekeeper[bot]

Please stay open.

Oct 17 '23 12:10 elfoxus

@hjohn Would you be willing to expand your feedback to a complete review?

Nov 14 '23 07:11 mstr2

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Dec 12 '23 17:12 bridgekeeper[bot]

@mstr2 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Jan 09 '24 17:01 bridgekeeper[bot]

/open

Mar 18 '24 17:03 mstr2

@mstr2 This pull request is now open

Mar 18 '24 17:03 openjdk[bot]

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

8311895: CSS Transitions

Reviewed-by: mmack, 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:

c8b96e4e6da18b4ab1331bb3a5b48cf1b8b3e2e6: 8333147: update maven classifier syntax to recent gradle version
4a33d5ef13c7bc72900fa6769fe4779ccec5bc98: 8330304: MenuBar: Invisible Menu works incorrectly with keyboard arrows
3e768dc78e68b8e2a9d39b72581ef2b1da0597b1: 8332748: Grammatical errors in animation API docs
a39652b3e72dd40dc2a24e06490d6b365f69bbe3: 8323511: Scrollbar Click jumps inconsistent amount of pixels

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.

Mar 18 '24 17:03 openjdk[bot]

I've implemented @hjohn's suggestion of an interface between TransitionTimer and StyleableProperty with the new TransitionMediator class. This cleans up the code and separates the interacting components.

This PR is now again ready for review.

Mar 18 '24 18:03 mstr2

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Apr 28 '24 14:04 bridgekeeper[bot]

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.

May 24 '24 11:05 mstr2

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:

Setting the property value
Binding the property
Making the node invisible (i.e. isTreeVisible() returns false)
Removing the node from the scene graph

In effect, transitions are only active when "left alone", any programmatic interaction will cancel them.

May 25 '24 21:05 mstr2

This is what I would expect, so looks good. Where is this mentioned to the user?

May 25 '24 21:05 nlisker

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.

May 25 '24 21:05 mstr2

I think the CSR needs updating.

May 26 '24 11:05 nlisker

jfx jfx copied to clipboard

8311895: CSS Transitions

Future enhancements

Limitations

Progress

Issues

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard