Skip to content
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.

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

8342703: CSS transition is not started when initial value was not specified #1607

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,10 @@ public static void reapplyCSS(Node node) {
nodeAccessor.reapplyCSS(node);
}

public static boolean isInitialCssState(Node node) {
return nodeAccessor.isInitialCssState(node);
}

public static void recalculateRelativeSizeProperties(Node node, Font fontForRelativeSizes) {
nodeAccessor.recalculateRelativeSizeProperties(node, fontForRelativeSizes);
}
Expand Down Expand Up @@ -391,6 +395,7 @@ boolean doComputeIntersects(Node node, PickRay pickRay,
void setLabeledBy(Node node, Node labeledBy);
Accessible getAccessible(Node node);
void reapplyCSS(Node node);
boolean isInitialCssState(Node node);
void recalculateRelativeSizeProperties(Node node, Font fontForRelativeSizes);
boolean isTreeVisible(Node node);
BooleanExpression treeVisibleProperty(Node node);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ public StyleableBooleanProperty(boolean initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, Boolean v) {
TransitionDefinition transition = this.origin != null && getBean() instanceof Node node ?
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition = getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

boolean newValue = v != null && v;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,8 @@ public StyleableDoubleProperty(double initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, Number v) {
// If this.origin == null, we're setting the value for the first time.
// No transition should be started in this case.
TransitionDefinition transition = this.origin != null && getBean() instanceof Node node ?
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition = getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

double newValue = v != null ? v.doubleValue() : 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ public StyleableFloatProperty(float initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, Number v) {
TransitionDefinition transition = this.origin != null && getBean() instanceof Node node ?
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition = getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

float newValue = v != null ? v.floatValue() : 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ public StyleableIntegerProperty(int initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, Number v) {
TransitionDefinition transition = this.origin != null && getBean() instanceof Node node ?
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition = getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

int newValue = v != null ? v.intValue() : 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,8 @@ public StyleableLongProperty(long initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, Number v) {
// If this.origin == null, we're setting the value for the first time.
// No transition should be started in this case.
TransitionDefinition transition = this.origin != null && getBean() instanceof Node node ?
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition = getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

long newValue = v != null ? v.longValue() : 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,9 @@ public void applyStyle(StyleOrigin origin, T newValue) {
* @param metadata the CSS metadata of the value
*/
private void applyValue(T oldValue, T newValue, CssMetaData<? extends Styleable, T> metadata) {
// If this.origin == null, we're setting the value for the first time.
// No transition should be started in this case.
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition =
this.origin != null && getBean() instanceof Node node ?
getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, metadata) : null;

// We only start a new transition if the new target value is different from the target
Expand Down Expand Up @@ -146,10 +145,9 @@ private void applyValue(T oldValue, T newValue, CssMetaData<? extends Styleable,
private void applyComponents(T oldValue, T newValue,
CssMetaData<? extends Styleable, T> metadata,
SubPropertyConverter<T> converter) {
// If this.origin == null, we're setting the value for the first time.
// No transition should be started in this case.
// If the value is applied for the first time, we don't start a transition.
Map<CssMetaData<? extends Styleable, ?>, TransitionDefinition> transitions =
this.origin != null && getBean() instanceof Node node ?
getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinitions(node, metadata) : null;

List<CssMetaData<? extends Styleable, ?>> subMetadata = metadata.getSubProperties();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@ public StyleableStringProperty(String initialValue) {
/** {@inheritDoc} */
@Override
public void applyStyle(StyleOrigin origin, String newValue) {
// If this.origin == null, we're setting the value for the first time.
// No transition should be started in this case.
// If the value is applied for the first time, we don't start a transition.
TransitionDefinition transition =
this.origin != null && getBean() instanceof Node node ?
getBean() instanceof Node node && !NodeHelper.isInitialCssState(node) ?
NodeHelper.findTransitionDefinition(node, getCssMetaData()) : null;

if (transition == null) {
Expand Down
32 changes: 32 additions & 0 deletions modules/javafx.graphics/src/main/java/javafx/scene/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,11 @@ public void reapplyCSS(Node node) {
node.reapplyCSS();
}

@Override
public boolean isInitialCssState(Node node) {
return node.initialCssState;
}

@Override
public void recalculateRelativeSizeProperties(Node node, Font fontForRelativeSizes) {
node.recalculateRelativeSizeProperties(fontForRelativeSizes);
Expand Down Expand Up @@ -1010,6 +1015,8 @@ protected void invalidated() {
}
updateDisabled();
computeDerivedDepthTest();
resetInitialCssStateFlag();

final Parent newParent = get();

// Update the focus bits before calling reapplyCss(), as the focus bits can affect CSS styling.
Expand Down Expand Up @@ -1104,8 +1111,14 @@ private void invalidatedScenes(Scene oldScene, SubScene oldSubScene) {
getClip().setScenes(newScene, newSubScene);
}
if (sceneChanged) {
if (oldScene != null) {
oldScene.unregisterClearInitialCssStageFlag(this);
}

if (newScene == null) {
completeTransitionTimers();
} else {
resetInitialCssStateFlag();
}
updateCanReceiveFocus();
if (isFocusTraversable()) {
Expand Down Expand Up @@ -10066,6 +10079,25 @@ private void doProcessCSS() {
}
}

/**
* A node is considered to be in its initial CSS state if it wasn't shown in a scene graph before.
* This flag is cleared after CSS processing was completed in a Scene pulse event. Note that manual
* calls to {@link #applyCss()} or similar methods will not clear this flag, since we consider all
* CSS processing before the Scene pulse to be part of the node's initial state.
*/
private boolean initialCssState = true;
mstr2 marked this conversation as resolved.
Show resolved Hide resolved

private void resetInitialCssStateFlag() {
initialCssState = true;
Scene scene = getScene();
if (scene != null) {
scene.registerClearInitialCssStateFlag(this);
}
}

void clearInitialCssStateFlag() {
initialCssState = false;
}

/**
* A StyleHelper for this node.
Expand Down
23 changes: 23 additions & 0 deletions modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,29 @@ private void doCSSPass() {
sceneRoot.clearDirty(com.sun.javafx.scene.DirtyBits.NODE_CSS);
sceneRoot.processCSS();
}

if (!clearInitialCssStateNodes.isEmpty()) {
for (var node : clearInitialCssStateNodes) {
node.clearInitialCssStateFlag();
}

clearInitialCssStateNodes.clear();
}
}

/**
* A list of nodes that have expressed their interest to be notified when the next CSS pass
* is completed. Nodes will use this event to determine whether they are in their initial
* CSS state (see {@link Node#initialCssState}.
*/
private final Set<Node> clearInitialCssStateNodes = Collections.newSetFromMap(new IdentityHashMap<>());

void registerClearInitialCssStateFlag(Node node) {
clearInitialCssStateNodes.add(node);
}

void unregisterClearInitialCssStageFlag(Node node) {
clearInitialCssStateNodes.remove(node);
}

void doLayoutPass() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ void setup() {
scene = new Scene(new Group());
stage = new Stage();
stage.setScene(scene);
stage.show();
}

@AfterEach
Expand All @@ -217,6 +216,7 @@ void testRedundantTransitionIsDiscarded(TestRun testRun) {
testRun.property.applyStyle(StyleOrigin.USER, testRun.defaultValue);
var mediator1 = getFieldValue(testRun.property, testRun.fieldName);
assertNull(mediator1);
stage.show();

// Start the transition. This adds it to the list of running transitions.
testRun.property.applyStyle(StyleOrigin.USER, testRun.newValue);
Expand All @@ -240,6 +240,7 @@ void testReversingTransitionIsNotDiscarded(TestRun testRun) {
testRun.property.applyStyle(StyleOrigin.USER, testRun.defaultValue);
var mediator1 = getFieldValue(testRun.property, testRun.fieldName);
assertNull(mediator1);
stage.show();

// Start the transition. This adds it to the list of running transitions.
testRun.property.applyStyle(StyleOrigin.USER, testRun.newValue);
Expand Down Expand Up @@ -271,6 +272,7 @@ void testExistingTransitionOfComponentTransitionableIsPreserved() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, border1);
stage.show();

// Start the transition and capture a copy of the sub-property mediator list.
// -fx-background-color will transition from RED to GREEN
Expand Down Expand Up @@ -301,6 +303,7 @@ void testIntegerTransitionsInRealNumberSpace() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, 0);
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, 2);
Expand All @@ -324,6 +327,7 @@ void testLongTransitionsInRealNumberSpace() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, 0);
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, 2);
Expand All @@ -347,6 +351,7 @@ void testBooleanTransitionsAsDiscrete() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, false);
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, true);
Expand All @@ -364,6 +369,7 @@ void testStringTransitionsAsDiscrete() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, "foo");
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, "bar");
Expand Down Expand Up @@ -403,6 +409,7 @@ enum Fruit { APPLE, ORANGE }
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, Fruit.APPLE);
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, Fruit.ORANGE);
Expand All @@ -427,6 +434,7 @@ void testNullObjectTransitionsAsDiscrete_withInterpolatableValue() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, Color.RED);
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, null);
Expand All @@ -451,6 +459,7 @@ void testNullObjectTransitionsAsDiscrete_withComponentTransitionableValue() {
// Setting a value for the first time doesn't start a transition.
setAnimationTime(0);
property.applyStyle(StyleOrigin.USER, Background.fill(Color.RED));
stage.show();

// Start the transition and sample the outputs.
property.applyStyle(StyleOrigin.USER, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ public void startup() {
scene = new Scene(new Group(node));
stage = new Stage();
stage.setScene(scene);
stage.show();
}

@AfterEach
Expand All @@ -89,7 +88,7 @@ public void testRegularPlayback() {
toolkit.setCurrentTime(0);
scene.getStylesheets().add(url);
node.getStyleClass().add("testClass");
node.applyCss();
stage.show();

List<TransitionEvent> trace = new ArrayList<>();
node.addEventHandler(TransitionEvent.ANY, trace::add);
Expand Down Expand Up @@ -132,7 +131,7 @@ public void testPlaybackIsElidedWhenDurationIsZero() {
toolkit.setCurrentTime(0);
scene.getStylesheets().add(url);
node.getStyleClass().add("testClass");
node.applyCss();
stage.show();

List<TransitionEvent> trace = new ArrayList<>();
node.addEventHandler(TransitionEvent.ANY, trace::add);
Expand Down Expand Up @@ -161,7 +160,7 @@ public void testInterruptedPlayback() {
toolkit.setCurrentTime(0);
scene.getStylesheets().add(url);
node.getStyleClass().add("testClass");
node.applyCss();
stage.show();

List<TransitionEvent> trace = new ArrayList<>();
node.addEventHandler(TransitionEvent.ANY, trace::add);
Expand Down Expand Up @@ -199,7 +198,7 @@ public void testInterruptedPlaybackWithNegativeDelay() {
toolkit.setCurrentTime(0);
scene.getStylesheets().add(url);
node.getStyleClass().add("testClass");
node.applyCss();
stage.show();

List<TransitionEvent> trace = new ArrayList<>();
node.addEventHandler(TransitionEvent.ANY, trace::add);
Expand Down Expand Up @@ -237,7 +236,7 @@ public void testInterruptedPlaybackDuringDelayPhase() {
toolkit.setCurrentTime(0);
scene.getStylesheets().add(url);
node.getStyleClass().add("testClass");
node.applyCss();
stage.show();

List<TransitionEvent> trace = new ArrayList<>();
node.addEventHandler(TransitionEvent.ANY, trace::add);
Expand Down
Loading