On Fri, 17 Jan 2020 14:09:54 GMT, Dean Wookey <dwoo...@openjdk.org> wrote:
> Everything passes with the fix and 5 of the new tests fail without the fix. > > removingThenAddingNodeToDifferentBranchGetsNewFontStyleTest > movingBranchToDifferentBranchGetsNewCssVariableTest > removingThenAddingNodeToDifferentBranchGetsCorrectInheritedValue > removingThenAddingNodeToDifferentBranchGetsIneritableStyle > movingNodeToDifferentBranchGetsNewFontStyleTest > > I doubt this will cause a significant performance decrease. I think it's > worth it for correctness. The only other thing is that I kept the fix to the > minimum, but technically canReuseHelper now has a side effect. I could try > rewrite some things if necessary, or maybe rename the method? Else leave it > as is? > > Originally introduced here: https://bugs.openjdk.java.net/browse/JDK-8090462/ > https://github.com/openjdk/jfx/commit/834b0d05e7a2a8351403ec4a121eab312d80fd24#diff-9ec098280fa1aeed53c70243347e76ab Changes requested by dgrieve (Reviewer). modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 134: > 133: node.styleHelper.triggerStates.addAll(triggerStates[0]); > 134: > 135: updateParentTriggerStates(node, depth, triggerStates); In canReuseStyleHelper, there is this check // If the style maps are the same instance, we can re-use the current styleHelper if the cacheContainer is null. // Under this condition, there are no styles for this node _and_ no styles inherit. if (node.styleHelper.cacheContainer == null) { return true; } And later in the same function is a check for (parent == null) that returns true. In both cases, firstStyleableAncestor will still point to the old first-styleable ancestor. ------------- PR: https://git.openjdk.java.net/jfx/pull/87
