8350917: Allow parent nodes to provide CSS styleable properties for child nodes #1714

hjohn · 2025-02-17T01:15:37Z

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)

Error

⚠️ The pull request body must not be empty.

Issue

JDK-8350917: Allow parent nodes to provide CSS styleable properties for child nodes (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1714

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-17T01:15:58Z

👋 Welcome back jhendrikx! 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.

openjdk · 2025-02-17T01:16:04Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

hjohn · 2025-02-18T11:13:14Z

@mstr2 / Dirk Lemmermann : This is a proof of concept PR for parent provided properties. I've adapted HBox / VBox only in this PR, and I probably need to update the CSS documentation still. I've opted to leave out the static HBox / VBox method to get these as a property (ie. hgrowProperty) although a property is used internally and so it could be exposed fairly easily later. My plan is:

Get some early feedback on the implementation, naming and the new API method in Styleable
Decide the scope of this PR; I'd be in favor to get the core code in, then add more support later to ease reviews
- Include new static property methods as well?
- Include GridPane as well?
- Any other controls?
Add some specific unit tests for this new feature
Update CSS docs
Ready for review
Integrate

openjdk · 2025-02-28T04:13:24Z

@hjohn Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2025-03-01T10:44:31Z

Webrevs

00: Full (433764cc)

kevinrushforth · 2025-03-03T21:17:19Z

/reviewer 2 reviewers
/csr

openjdk · 2025-03-03T21:18:21Z

@kevinrushforth Only the author (@hjohn) is allowed to issue the reviewer command.

openjdk · 2025-03-03T21:18:30Z

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

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

hjohn · 2025-03-03T21:32:51Z

/reviewer 2 reviewers

hjohn · 2025-03-03T21:33:11Z

Only the author (@hjohn) is allowed to issue the reviewer command.

That's weird isn't it?

openjdk · 2025-03-03T21:33:56Z

@hjohn Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

/reviewer credit @openjdk-bot
/reviewer credit duke
/reviewer credit @user1 @user2

hjohn · 2025-03-03T21:34:45Z

/reviewers 2 reviewers

openjdk · 2025-03-03T21:35:02Z

@hjohn
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

kevinrushforth · 2025-03-03T22:49:23Z

Well, oops.

Only the author (@hjohn) is allowed to issue the reviewer command.

That's weird isn't it?

No, it was my mistake. There are two different Skara commands with two different meanings and very similar spelling:

/reviewers -- set the minimum number of reviewers needed to approve
/reviewer -- credit extra reviewers who didn't "review" in the GitHub sense, but whom you want to credit in the meta-data (they don't count towards the minimum number of reviewers)

I meant the first one. I accidentally typed the second.

hjohn · 2025-03-11T20:21:02Z

@mstr2 could you have a look?

mstr2 · 2025-03-16T17:08:31Z

modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java

@@ -461,6 +462,8 @@ private StyleMap getStyleMap(Styleable styleable) {
        private boolean forceSlowpath = false;
    }

+    private record StylingContext(Node node, CalculatedValue font, StyleMap styleMap, Set<PseudoClass> pseudoClasses) {}


It might not be a good trade-off to create lots of transient objects on a hot path just to save a few arguments in the calling convention. This would be a nice improvement once we have value types in Java.

I checked this before hand, there is quite a bit more going on in creating the cache keys (see getTransitionStates), and I think this extra object will therefore be lost in the noise.

I still think that it doesn’t carry its weight (bundling up private method arguments in the same translation unit is not a big win). It will increase churn on the GC, even if slightly, and many of these decisions taken together may cause it to run more often or earlier.

mstr2 · 2025-03-16T18:36:05Z

modules/javafx.graphics/src/main/java/javafx/css/Styleable.java

+     * Note: this method does not provide CSS meta data for <b>this</b> Styleable,
+     * only for its direct children!
+     *
+     * @return an immutable list of CSS meta data, never {@code null}


Since we don't specify that getCssMetaData() should never return null, we should consider also not specifying it here. Alternatively, maybe we should add the non-null requirement to the existing getCssMetaData().

That might be a good idea, as there are several places in CssStyleHelper where getCssMetaData is assumed to be non-null already (and also a few locations where it is assumed it can be null). In other words, if the function would return null, FX would break currently.

For example, recalculateRelativeSizeProperties will do:

final List<CssMetaData<? extends Styleable, ?>> styleables = node.getCssMetaData(); final int numStyleables = styleables.size();

Which would be an NPE.

I think then you can do this as part of this PR, since you’re already touching the Styleable interface.

mstr2 · 2025-03-16T18:53:41Z

modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java

+            name = name.substring(4);
+        }
+
+        String propertyName = name + " (parent property)";


I think a more sensible naming scheme would be something like "VBox.margin", which should be supplied as an argument and not generated on the fly.

I can change that to whatever you like. Be aware this is only for the toString (both getBean and getName are optional methods to implement).

mstr2 · 2025-03-16T18:56:54Z

modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java

+
+        @SuppressWarnings("unchecked")
+        StyleableObjectProperty<T> castProperty = (StyleableObjectProperty<T>) child.getProperties()
+            .computeIfAbsent(cssMetaData, k -> createChildConstraintProperty(child, cssMetaData));


Since we're on a hot path here, I'd like to see this work without unnecessary memory allocations:

Check hasProperties() before calling getProperties().

Don't use computeIfAbsent() since it requires a capturing lambda.

mstr2 · 2025-03-16T19:03:32Z

modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java

+                return castProperty.getValue();
+            }
+
+            if (value != null) {


How would the value ever be anything other than a StyleableObjectProperty, since setChildConstraint() always creates a StyleableObjectProperty?

Yeah, this is not handled well. It was intended to mimic a standard value with the property being created (like the lazy property pattern), but it doesn't work here.

In theory, as the property map is public, one could set the key to something else (not easily mind you, you'd need to iterate to find the keys as they're no longer predictable Strings). So, I'll remove this code, and just ignore any bad value and proceed with returning the default value.

mstr2 · 2025-03-17T06:29:28Z

Your approach for this enhancement looks good. Do you plan to add styleable properties for the other layout containers too?

hjohn · 2025-03-17T09:18:56Z

Your approach for this enhancement looks good. Do you plan to add styleable properties for the other layout containers too?

I would like to, I was thinking of doing that in a separate PR as there are quite a few (and I think they would need documentation as well). I think there's GridPane, StackPane, AnchorPane, TilePane and BorderPane as well. It might speed along reviews to keep the PR's smaller.

bridgekeeper · 2025-04-14T15:20:19Z

@hjohn 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!

bridgekeeper · 2025-05-12T20:26:32Z

@hjohn 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.

hjohn · 2025-07-06T10:23:26Z

This would also address https://bugs.openjdk.org/browse/JDK-8092347

hjohn · 2025-07-06T10:23:50Z

/open

openjdk · 2025-07-06T10:23:58Z

@hjohn This pull request is now open

hjohn force-pushed the feature/parent-styleable-properties branch from ff29324 to e80c611 Compare February 18, 2025 00:37

hjohn marked this pull request as ready for review February 26, 2025 13:01

hjohn changed the title ~~Proof of concept parent provided styleable properties~~ JDK-8350917 Allow parent nodes to provide CSS styleable properties for child nodes Feb 28, 2025

openjdk bot changed the title ~~JDK-8350917 Allow parent nodes to provide CSS styleable properties for child nodes~~ 8350917: Allow parent nodes to provide CSS styleable properties for child nodes Feb 28, 2025

Proof of concept parent provided styleable properties

38e77a9

hjohn force-pushed the feature/parent-styleable-properties branch from e80c611 to 38e77a9 Compare February 28, 2025 04:13

Remove white space

433764c

openjdk bot added the rfr Ready for review label Mar 1, 2025

openjdk bot added the csr Need approved CSR to integrate pull request label Mar 3, 2025

mstr2 reviewed Mar 16, 2025

View reviewed changes

bridgekeeper bot closed this May 12, 2025

openjdk bot reopened this Jul 6, 2025

openjdk bot removed the rfr Ready for review label Jul 17, 2025

8350917: Allow parent nodes to provide CSS styleable properties for child nodes #1714

Are you sure you want to change the base?

8350917: Allow parent nodes to provide CSS styleable properties for child nodes #1714

Uh oh!

Conversation

hjohn commented Feb 17, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Error

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Feb 17, 2025

Uh oh!

openjdk bot commented Feb 17, 2025

Uh oh!

hjohn commented Feb 18, 2025

Uh oh!

openjdk bot commented Feb 28, 2025

Uh oh!

mlbridge bot commented Mar 1, 2025

Webrevs

Uh oh!

kevinrushforth commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025

Uh oh!

hjohn commented Mar 3, 2025

Uh oh!

hjohn commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025

Uh oh!

hjohn commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025

Uh oh!

kevinrushforth commented Mar 3, 2025

Uh oh!

hjohn commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Mar 17, 2025

Uh oh!

hjohn commented Mar 17, 2025

Uh oh!

bridgekeeper bot commented Apr 14, 2025

Uh oh!

bridgekeeper bot commented May 12, 2025

Uh oh!

hjohn commented Jul 6, 2025

Uh oh!

hjohn commented Jul 6, 2025

Uh oh!

openjdk bot commented Jul 6, 2025

Uh oh!

Uh oh!

hjohn commented Feb 17, 2025 •

edited by openjdk bot

Loading