diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
index 519f7e3a688..223d4264697 100644
--- a/modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
+++ b/modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
@@ -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.
+ *
+ * {@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).
+ *
+ * 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.
+ *
+ * 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;
@@ -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) {
diff --git a/modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java b/modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java
index 311cbae416a..2cd3b105244 100644
--- a/modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java
+++ b/modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java
@@ -140,8 +140,8 @@ public void set(S styleable, V value, StyleOrigin origin) {
/**
* Check to see if the corresponding property on the given Node is
- * settable. This method is called before any styles are looked up for the
- * given property. It is abstract so that the code can check if the property
+ * settable. This method is checked before calling {@link StyleableProperty#applyStyle(javafx.css.StyleOrigin, java.lang.Object)}
+ * for the given property. 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.
*
diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
index 75b02c27c63..888f86fd76a 100644
--- a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
@@ -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]);
@@ -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 cssSetProperties;
-
- private boolean forceSlowpath = false;
}
private boolean resetInProgress = false;
@@ -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);
@@ -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;
@@ -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);
- // 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);
@@ -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;
+
// 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.
+ */
+
+ 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);
}
@@ -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
diff --git a/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java b/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
index a2e4f46440d..f4e77be8913 100644
--- a/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
+++ b/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
@@ -28,16 +28,27 @@
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Base64;
+import java.util.Collections;
import java.util.List;
+import java.util.Map;
import com.sun.javafx.css.StyleManager;
import com.sun.javafx.tk.Toolkit;
import javafx.application.ColorScheme;
+import javafx.beans.property.SimpleDoubleProperty;
+import javafx.beans.property.SimpleObjectProperty;
+import javafx.css.CssMetaData;
import javafx.css.CssParser;
import javafx.css.CssParser.ParseError;
import javafx.css.CssParser.ParseError.PropertySetError;
import javafx.css.PseudoClass;
+import javafx.css.Styleable;
+import javafx.css.StyleableDoubleProperty;
+import javafx.css.StyleableProperty;
+import javafx.css.StyleConverter;
+import javafx.css.StyleOrigin;
import javafx.css.Stylesheet;
+import javafx.css.converter.SizeConverter;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.layout.Background;
@@ -676,6 +687,278 @@ public void initialNodeWithUserSetValueShouldNotResetValuesOnOtherNodesWithoutOv
assertEquals(new Insets(4), b.getPadding());
}
+ @Test
+ public void boundPropertyShouldNotAdverselyAffectUnrelatedNode() throws IOException {
+
+ /*
+ * Create a stylesheet that applies padding unconditionally, and a test pseudo-class
+ * that can be used to trigger a CSS state transition:
+ */
+
+ Stylesheet stylesheet = new CssParser().parse(
+ "boundPropertyShouldNotAdverselyAffectUnrelatedNode",
+ """
+ .pane {
+ -fx-padding: 4;
+ }
+ .pane:test-marker {
+ -fx-opacity: 0.5;
+ }
+ """
+ );
+
+ StyleManager.getInstance().setDefaultUserAgentStylesheet(stylesheet);
+ Pane bound = new Pane();
+ Pane other = new Pane();
+
+ bound.getStyleClass().add("pane");
+ other.getStyleClass().add("pane");
+
+ root.getChildren().addAll(bound, other);
+ stage.show();
+
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(new Insets(4), bound.getPadding());
+ assertEquals(new Insets(4), other.getPadding());
+
+ /*
+ * Bind the padding property, so CSS can no longer apply a value to it directly:
+ */
+
+ bound.paddingProperty().bind(new SimpleObjectProperty<>(new Insets(99)));
+
+ /*
+ * Transition to a state that has not yet been seen before, so a new shared style
+ * cache entry is created. "bound" is the first node to reach it: it cannot apply
+ * -fx-padding to itself, but it should still resolve and cache the correct value for
+ * whoever else shares this entry:
+ */
+
+ bound.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ /*
+ * "other" now transitions into that same state and takes the fast path, reusing the
+ * value "bound" already resolved, and keeps its correctly styled padding:
+ */
+
+ other.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(new Insets(4), other.getPadding());
+ }
+
+ @Test
+ public void lateCssMetaDataShouldNotAdverselyAffectUnrelatedNode() throws IOException {
+
+ /*
+ * Create a stylesheet that applies -fx-extra unconditionally, and a test pseudo-class
+ * that can be used to trigger a CSS state transition:
+ */
+
+ Stylesheet stylesheet = new CssParser().parse(
+ "lateCssMetaDataShouldNotAdverselyAffectUnrelatedNode",
+ """
+ .pane {
+ -fx-extra: 40;
+ }
+ .pane:test-marker {
+ -fx-opacity: 0.5;
+ }
+ """
+ );
+
+ StyleManager.getInstance().setDefaultUserAgentStylesheet(stylesheet);
+
+ DummyControl late = new DummyControl();
+ DummyControl other = new DummyControl();
+
+ other.attachExtraProperty();
+
+ /*
+ * The "other" dummy control has the extra property attached to it immediately
+ * so CSS will see it from the start. The "late" variant never attaches the
+ * extra property, and so its CSS metadata list lacks it. This is similar to
+ * what happens when a control is added to a scene during layout; it won't have
+ * its skin applied yet, and so its CSS metadata may be incomplete.
+ */
+
+ late.getStyleClass().add("pane");
+ other.getStyleClass().add("pane");
+
+ root.getChildren().addAll(late, other);
+
+ stage.show();
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(40.0, other.getExtra());
+
+ /*
+ * The "late" control is the first node that gets the test-marker state, which
+ * has not been seen before. Because its CSS metadata does not include
+ * -fx-extra, it can't compute a value for it to be stored in the cache.
+ * This is okay, as the CSS engine should always recompute values not explicitly
+ * part of the cache, instead of assuming the absence of a value has any meaning:
+ */
+
+ late.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ /*
+ * Transition the "other" control now to the same state, and check if it did
+ * not revert or otherwise corrupt the -fx-extra property:
+ */
+
+ other.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(40.0, other.getExtra());
+ }
+
+ @Test
+ public void exceptionDuringApplyShouldNotPreventResetOfUnrelatedNode() throws IOException {
+
+ /*
+ * Create a stylesheet that uses a style that can throw an exception when
+ * evaluated, and a test pseudo-class that can be used to trigger a CSS state transition:
+ */
+
+ Stylesheet stylesheet = new CssParser().parse(
+ "exceptionDuringApplyShouldNotPreventResetOfUnrelatedNode",
+ """
+ .pane {
+ -fx-boom: 40;
+ }
+ .pane:test-marker {
+ -fx-boom: 41;
+ }
+ """
+ );
+
+ StyleManager.getInstance().setDefaultUserAgentStylesheet(stylesheet);
+
+ ExceptionalPropertyControl throwing = new ExceptionalPropertyControl();
+ ExceptionalPropertyControl other = new ExceptionalPropertyControl();
+
+ throwing.getStyleClass().add("pane");
+ other.getStyleClass().add("pane");
+
+ root.getChildren().addAll(throwing, other);
+
+ stage.show();
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(40.0, other.getBoom());
+
+ /*
+ * Make the throwing control throw an exception whenever CSS next tries to apply -fx-boom to it:
+ */
+
+ throwing.explodeOnApply = true;
+
+ /*
+ * The throwing node is the first to transition to the test-marker state, that hasn't
+ * been seen before. Even though evaluating -fx-boom will throw an exception, it should
+ * cache the result as SKIP:
+ */
+
+ throwing.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ /*
+ * Normally, on an exception, the CSS engine will reset the value back to its initial
+ * value as well, but as the test class will reject that as well with an exception,
+ * it will remain as is:
+ */
+
+ assertEquals(40.0, throwing.getBoom());
+
+ /*
+ * The other node is now transitioned to the same state. Since the value of -fx-boom
+ * could not be evaluated, the value is reset to its default value (1.0).
+ */
+
+ other.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(1.0, other.getBoom());
+
+ // disable the exception as other tests still use the same stage:
+ throwing.explodeOnApply = false;
+ }
+
+ @Test
+ public void boundPropertyIsStillComputedAndCachedForUnrelatedNode() throws IOException {
+
+ /*
+ * Create a stylesheet that uses a style that can be counted, and a
+ * test pseudo-class that can be used to trigger a CSS state transition:
+ */
+
+ Stylesheet stylesheet = new CssParser().parse(
+ "boundPropertyIsStillComputedAndCachedForUnrelatedNode",
+ """
+ .pane {
+ -fx-extra-value: 40;
+ }
+ .pane:test-marker {
+ -fx-opacity: 0.5;
+ }
+ """
+ );
+
+ StyleManager.getInstance().setDefaultUserAgentStylesheet(stylesheet);
+
+ DummyControl bound = new DummyControl();
+ DummyControl other = new DummyControl();
+
+ bound.attachExtraProperty();
+ other.attachExtraProperty();
+
+ bound.getStyleClass().add("pane");
+ other.getStyleClass().add("pane");
+
+ root.getChildren().addAll(bound, other);
+
+ stage.show();
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(40.0, bound.getExtra());
+ assertEquals(40.0, other.getExtra());
+
+ /*
+ * Make sure that the bound control can't apply a value for -fx-extra to itself:
+ */
+
+ bound.extra.bind(new SimpleDoubleProperty(99));
+
+ int countBeforeTransition = DummyControl.conversionCount;
+
+ /*
+ * The bound control is now transitioned to the test-marker pseudo-class, which
+ * has not yet been seen before and so a new cache entry must be created. Even
+ * though it is bound, it can still compute a useful value for the cache immediately:
+ */
+
+ bound.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(countBeforeTransition + 1, DummyControl.conversionCount);
+
+ /*
+ * The other control is now transitioned into that same state and should use
+ * the cache. No additional computation is expected, and so further conversion
+ * call should happen:
+ */
+
+ other.pseudoClassStateChanged(PseudoClass.getPseudoClass("test-marker"), true);
+ Toolkit.getToolkit().firePulse();
+
+ assertEquals(countBeforeTransition + 1, DummyControl.conversionCount);
+ assertEquals(40.0, other.getExtra());
+ }
+
@Test
public void shouldDetectSimpleInfiniteLoop() throws IOException {
Stylesheet stylesheet = new CssParser().parse(
@@ -968,4 +1251,138 @@ public void mediaQueryRemovalShouldNotInterruptTransitionsDuringReset() {
private static String toDataURL(String stylesheet) {
return "data:text/plain;base64," + Base64.getEncoder().encodeToString(stylesheet.getBytes(StandardCharsets.UTF_8));
}
+
+ /**
+ * A Pane subclass with an optional extra styleable property, "-fx-extra".
+ *
+ * This class is used to simulate what happens when a skin is set on a {@code Control}:
+ * Its {@code getCssMetaData()} will change values, exposing new not seen before
+ * CSS properties that were supplied by the skin.
+ *
+ * It can also be used to count how many times the converter is called for this property,
+ * from which we can infer if the value was actually evaluated (despite being bound for
+ * example). This is done with a sub-property trick.
+ */
+ private static final class DummyControl extends Pane {
+
+ private static int conversionCount;
+
+ private static final CssMetaData EXTRA_VALUE =
+ new CssMetaData<>("-fx-extra-value", SizeConverter.getInstance(), 1.0) {
+ @Override public boolean isSettable(DummyControl node) { return false; }
+ @Override public StyleableProperty getStyleableProperty(DummyControl node) { return null; }
+ };
+
+ private static final StyleConverter COUNTING_CONVERTER = new StyleConverter<>() {
+ @Override
+ public Number convert(Map, Object> convertedValues) {
+ conversionCount++;
+
+ return (Number)convertedValues.get(EXTRA_VALUE);
+ }
+ };
+
+ private static final CssMetaData EXTRA =
+ new CssMetaData<>("-fx-extra", COUNTING_CONVERTER, 1.0, false, List.of(EXTRA_VALUE)) {
+ @Override
+ public boolean isSettable(DummyControl node) {
+ return !node.extra.isBound();
+ }
+
+ @Override
+ public StyleableProperty getStyleableProperty(DummyControl node) {
+ return node.extra;
+ }
+ };
+
+ private final StyleableDoubleProperty extra = new StyleableDoubleProperty(1.0) {
+ @Override public Object getBean() { return DummyControl.this; }
+ @Override public String getName() { return "extra"; }
+ @Override public CssMetaData getCssMetaData() { return EXTRA; }
+ };
+
+ private boolean extraAttached;
+ private List> cachedMetaData;
+
+ void attachExtraProperty() {
+ extraAttached = true;
+ cachedMetaData = null;
+ }
+
+ double getExtra() {
+ return extra.get();
+ }
+
+ @Override
+ public List> getCssMetaData() {
+ if (cachedMetaData == null) {
+ List> list = new ArrayList<>(Pane.getClassCssMetaData());
+
+ if (extraAttached) {
+ list.add(EXTRA);
+ }
+
+ cachedMetaData = Collections.unmodifiableList(list);
+ }
+
+ return cachedMetaData;
+ }
+ }
+
+ /**
+ * A Pane subclass with one extra styleable property, "-fx-boom". Its applyStyle()
+ * can throw an exception on demand. This can be used to check if the CSS engine
+ * always correctly stores a SKIP entry in the cache for properties it couldn't
+ * evaluate.
+ */
+ private static final class ExceptionalPropertyControl extends Pane {
+
+ private static final CssMetaData BOOM =
+ new CssMetaData<>("-fx-boom", SizeConverter.getInstance(), 1.0) {
+ @Override
+ public boolean isSettable(ExceptionalPropertyControl node) {
+ return !node.boom.isBound();
+ }
+
+ @Override
+ public StyleableProperty getStyleableProperty(ExceptionalPropertyControl node) {
+ return node.boom;
+ }
+ };
+
+ private static final List> STYLEABLES;
+
+ static {
+ List> styleables = new ArrayList<>(Pane.getClassCssMetaData());
+
+ styleables.add(BOOM);
+
+ STYLEABLES = Collections.unmodifiableList(styleables);
+ }
+
+ private boolean explodeOnApply;
+
+ private final StyleableDoubleProperty boom = new StyleableDoubleProperty(1.0) {
+ @Override public Object getBean() { return ExceptionalPropertyControl.this; }
+ @Override public String getName() { return "boom"; }
+ @Override public CssMetaData getCssMetaData() { return BOOM; }
+
+ @Override public void applyStyle(StyleOrigin origin, Number value) {
+ if (explodeOnApply) {
+ throw new RuntimeException("boom!");
+ }
+
+ super.applyStyle(origin, value);
+ }
+ };
+
+ double getBoom() {
+ return boom.get();
+ }
+
+ @Override
+ public List> getCssMetaData() {
+ return STYLEABLES;
+ }
+ }
}