Skip to content

8388378: [Regression] FX CSS: styled property is reset to its initial value on a pseudo-class-only transition after tree items change (TreeTableRow -fx-indent collapses) #2218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hjohn wants to merge 4 commits into from
Closed

8388378: [Regression] FX CSS: styled property is reset to its initial value on a pseudo-class-only transition after tree items change (TreeTableRow -fx-indent collapses) #2218

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c52fd0
Fix CssStyleHelper treating absent cache values as "no style applies"
hjohn Jul 19, 2026
8739699
Add more information about how the CSS information works...
hjohn Jul 23, 2026
5dcd54c
Fix typos
hjohn Jul 23, 2026
905bda3
Update javadoc
hjohn Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,37 @@
import java.util.Set;

/**
*
* Caches the calculated value of each styleable property a specific combination of
* pseudo-class states and font size (see {@link Key}). An entry is shared by every
* {@code Styleable} that has that same combination.
* <p>
* {@link #get(String)} returning {@code null} means only that no one has evaluated that
* property for this entry yet; it should never be treated as "no style applies"(!). This is
* because when an entry is absent, it could be because the property was bound, or the property
* didn't exist at the time of evaluation (because a Skin with that property wasn't attached yet
* or Skins were swapped).
* <p>
* Therefore, callers must always resolve a miss with a real lookup before trusting the result; only
* a value actually stored via {@link #put(String, CalculatedValue)} can be trusted to be reused.
* <p>
* Entries are (currently) keyed purely by property name, with no check that a cached value's type
* still matches what the current lookup expects. This is harmless as long as every {@code
* Styleable} that could share an entry agrees on the type behind a given property name (which
* is generally standard for CSS properties). It is not a problem for two different {@code
* CssMetaData} instances to have the same property name, only if their types would also differ.
*/
public final class StyleCacheEntry {

public StyleCacheEntry() {
}

/**
* Returns the {@link CalculatedValue} for the given property, or {@code null}
* if none was cached.
*
* @param property a property to look up, cannot be {@code null}
* @return a {@link CalculatedValue}, or {@code null} if none has been cached yet
*/
public CalculatedValue get(String property) {

CalculatedValue cv = null;
Expand All @@ -49,6 +73,12 @@ public CalculatedValue get(String property) {
return cv;
}

/**
* Stores the given {@link CalculatedValue} for a property.
*
* @param property a property to store a {@link CalculatedValue} for, cannot be {@code null}
* @param calculatedValue a {@link CalculatedValue} to store, cannot be {@code null}
*/
public void put(String property, CalculatedValue calculatedValue) {

if (calculatedValues == null) {
Expand Down
76 changes: 26 additions & 50 deletions modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ static CssStyleHelper createStyleHelper(final Node node) {
if (node.styleHelper.cacheContainer != null && node.styleHelper.isUserSetFont(node)) {
node.styleHelper.cacheContainer.fontSizeCache.clear();
}
node.styleHelper.cacheContainer.forceSlowpath = true;

if (triggerStates[0] != null) {
node.styleHelper.triggerStates.addAll(triggerStates[0]);
Expand Down Expand Up @@ -460,8 +459,6 @@ private StyleMap getStyleMap(Styleable styleable) {
// here so the property can be reset without expanding properties that
// were not set by css.
private final Map<CssMetaData, CalculatedValue> cssSetProperties;

private boolean forceSlowpath = false;
}

private boolean resetInProgress = false;
Expand Down Expand Up @@ -890,9 +887,6 @@ void transitionToState(final Node node) {
final StyleCacheEntry.Key cacheEntryKey = new StyleCacheEntry.Key(transitionStates, fontForRelativeSizes);
StyleCacheEntry cacheEntry = sharedCache.getStyleCacheEntry(cacheEntryKey);

// if the cacheEntry already exists, take the fastpath
final boolean fastpath = cacheEntry != null;

if (cacheEntry == null) {
cacheEntry = new StyleCacheEntry();
sharedCache.addStyleCacheEntry(cacheEntryKey, cacheEntry);
Expand All @@ -903,9 +897,6 @@ void transitionToState(final Node node) {
// Used in the for loop below, and a convenient place to stop when debugging.
final int max = styleables.size();

final boolean isForceSlowpath = cacheContainer.forceSlowpath;
cacheContainer.forceSlowpath = false;

// For each property that is settable, we need to do a lookup and
// transition to that value.
transitionStateInProgress = true;
Expand All @@ -923,33 +914,18 @@ void transitionToState(final Node node) {
continue;
}

// Skip the lookup if we know there isn't a chance for this property
// to be set (usually due to a "bind").
if (!cssMetaData.isSettable(node)) continue;

final String property = cssMetaData.getProperty();

CalculatedValue calculatedValue = cacheEntry.get(property);

@andy-goryachev-oracle andy-goryachev-oracle Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the value is stored by property name, which means if the SKIP value was stored and then the metadata was replaced (by setting a skin, for example, which uses the same property names), the old SKIP would prevent re-evaluation.

@hjohn hjohn Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A property always evaluates the same, as this is dictated by the stylesheet, not by the presence or absence of a property in the CSS metadata. So the stylesheet says: xyz must be 5 here, and if you replace the skin and it has the same property, that evaluation is still correct.

There may be a problem if that new property is of a different type (I didn't test), but I consider that beyond the scope of this fix as that has never worked. If you see an easy solution, I can consider adding it -- my primary goal was to fix the customer found regression that seemed to have been due to #1076 but in reality has been there much longer, only less visible.

@andy-goryachev-oracle andy-goryachev-oracle Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit it's a rather contrived scenario, so I am not concerned. The devs can always work around this by using different property names.

Maybe just add a comment pointing to this scenario?

@hjohn hjohn Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to StyleCacheEntry:

Entries are (currently) keyed purely by property name, with no check that a cached value's type still matches what the current lookup expects. This is harmless as long as every {@code Styleable} that could share an entry agrees on the type behind a given property name (which is generally standard for CSS properties). It is not a problem for two different {@code CssMetaData} instances to have the same property name, only if their types would also differ.


// If there is no calculatedValue and we're on the fast path,
// take the slow path if cssFlags is REAPPLY (JDK-8116341)
final boolean forceSlowpath =
fastpath && calculatedValue == null && isForceSlowpath;

final boolean addToCache =
(!fastpath && calculatedValue == null) || forceSlowpath;

if (fastpath && !forceSlowpath) {

// If the cache contains SKIP, then there was an
// exception thrown from applyStyle
if (calculatedValue == SKIP) {
continue;
}
if (calculatedValue == null) {

} else if (calculatedValue == null) {
/*
* A cache miss occurred; this means that either we're the first to evaluate
* this property, or that the CssMetaData didn't include this property yet
* (not all styleables have stable CssMetaData, most notably Control).
*/

// slowpath!
calculatedValue = lookup(node, cssMetaData, styleMap, transitionStates[0],
node, cachedFont);

Expand All @@ -959,33 +935,40 @@ void transitionToState(final Node node) {
continue;
}

cacheEntry.put(property, calculatedValue);
}

/*
* Skip this property (after caching) if it can't be set (usually because it is bound).
* The cached value is still useful for others sharing this entry.
*/

if (!cssMetaData.isSettable(node)) continue;

@Maran23 Maran23 Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer this style, but again, minor and is all of the place in this file anyway

Suggested change
if (!cssMetaData.isSettable(node)) continue;
if (!cssMetaData.isSettable(node)) {
continue;
}

@andy-goryachev-oracle andy-goryachev-oracle Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the contract for CssMetaData.isSettable() says "This method is called before any styles are looked up for the given property." but here it's called after the lookup() in L929

@hjohn hjohn Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an odd amount of detail that is rather implementation specific.

I would suggest we remove it, as I don't think there are good reasons for this other than "that's how it works currently", and it actually is an active source of problems.

A property being unsettable does not mean you can't evaluate the stylesheet to find what value it would have gotten (and that's what we do now). The only reason this method exists is that is faster than catching "RuntimeException: A bound value cannot be set.".

There are two good reasons to still evaluate the CSS value (even if we won't set it):

  • The property may not be bound forever; if the cache entry was created without the calculation of the value for a "locked" property, then we later won't know what CSS based value to put in there (note: unbinding a property is not detected, so binding/unbinding already plays very badly with the CSS system).

  • The cache entry may be shared with siblings in the same state; if the cache entry was initialized by a Node with bound properties, and we then just skip those, then the other siblings would not have that value either -- this causes incorrect value resets and properties to not be styled, which this PR specifically wants to address

If there are good reasons to keep it, then the only other option we have is to NOT create a cache entry at all when a Node has any CSS property bound (a sibling that is in a better state may create it still though). This could be a performance hazard though, because if it is the only node of its kind, it would always need to go through the slow lookup path until there are no more bound properties.

@Maran23 Maran23 Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for removing it.

@andy-goryachev-oracle andy-goryachev-oracle Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean removing the spec "This method is called before any styles are looked up for the given property." ?

@hjohn hjohn Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't think that should have been in the spec in the first place, as it reveals basically the inner workings of the CSS system. Also, we're still respecting the return value of isSettable -- we're not calling applyStyle (which would indeed result in the "expansion of the property" the documentation alludes to). We're merely looking up the styles, something the documentation specifically says it won't do -- but why guarantee that in the first place?

So I suggest rewriting the docs to:

     * Check to see if the corresponding property on the given Node is
     * settable. This method is called before any call to {@code applyStyle} is
     * made. It is abstract so that the code can check if the property
     * is settable without expanding the property. Generally, the property is
     * settable if it is not null or is not bound.

Changing the sentence "This method is called before any styles are looked up for the given property." to "This method is called before any call to {@code applyStyle} is made." -- in both cases this ensures the property is not expanded unnecessarily, but gives the CSS system the freedom to still evaluate the stylesheet so it can create a shared cache entry that is complete.


// StyleableProperty#applyStyle might throw an exception and it is called
// from two places in this try block.
try {

//
// JDK-8127435
// If the current value of the property was set by CSS
// and there is no style for the property, then reset this
// property to its initial value. If it was not set by CSS
// then leave the property alone.
//
if (calculatedValue == null || calculatedValue == SKIP) {
/*
* JDK-8127435: If there is no style for the property (SKIP), then check if it must be reset
* to its initial value. Otherwise, continue with CSS application.
*/

@andy-goryachev-oracle andy-goryachev-oracle Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I would have used // style comments


if (calculatedValue == SKIP) { // calculatedValue is never null here

// cssSetProperties keeps track of the StyleableProperty's that were set by CSS in the previous state.
// If this property is not in cssSetProperties map, then the property was not set in the previous state.
// This accomplishes two things. First, it lets us know if the property was set in the previous state
// so it can be reset in this state if there is no value for it. Second, it calling
// so it can be reset in this state if there is no value for it. Second, it avoids calling
// CssMetaData#getStyleableProperty which is rather expensive as it may cause expansion of lazy
// properties.
CalculatedValue initialValue = cacheContainer.cssSetProperties.get(cssMetaData);

// if the current value was set by CSS and there
// is no calculated value for the property, then
// there was no style for the property in the current
// state, so reset the property to its initial value.
/*
* If the initial value is not null, then the property was set by CSS
* on this node, and so it must be reset:
*/

if (initialValue != null) {
resetToInitialValue(node, cssMetaData, initialValue);
}
Expand All @@ -994,13 +977,6 @@ void transitionToState(final Node node) {

}

if (addToCache) {

// If we're not on the fastpath, then add the calculated
// value to cache.
cacheEntry.put(property, calculatedValue);
}

StyleableProperty styleableProperty = cssMetaData.getStyleableProperty(node);

// need to know who set the current value - CSS, the user, or init
Expand Down
Loading