8388277: [REDO] Looked-up color fails for -fx-background-color in JavaFX CSS file - #2225
Conversation
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Maran23 |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
please resolve the merge conflict.
…REDO]-Looked-up-color-fails-for--fx-background-color-in-JavaFX-CSS-file # Conflicts: # modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
|
Our project JabRef is currently dependent on this fix for a critical performance bug report. |
GIven that the earlier version caused a regression, and that this touches an area of the code that is particularly tricky, it will need sufficient time to review and test it. It will then go into jfx28 and is unlikely to be backported, given the risk, at least not right away. |
| */ | ||
| private static boolean isStyleableAncestor(Node parentNode) { | ||
| if (parentNode.cssHelperState == Node.CssHelperState.STALE) { | ||
| parentNode.cssHelperState = Node.CssHelperState.RESOLVED_EARLY; |
There was a problem hiding this comment.
there might be a problem here: this code sets RESOLVED_EARLY before creating the styleHelper. createStyleHelper() can invoke application listeners that might, for example, add new nodes and force applyCss() immediately. The re-entrance descendants see RESOLVED_EARLY and trust the old, stale, styleHelper.
would it make sense to add another state, or perhaps move the state into styleHelper ?
| Styleable parent = node; | ||
| int depth = 0; | ||
| while(parent != null) { | ||
| while (parent != null) { |
There was a problem hiding this comment.
there might be potential quadratic execution time here.
imagine a long chain of nodes
root
+ node1
+ node2
...
+ nodeN
- start by adding a child to nodeN, this makes the entire chain dirty.
- change style class in all the nodes in the chain
- add a second child to nodeN
adding the second child recursively rebuilds every stale ancestor, walking all the way to the root.
| } | ||
|
|
||
| if (node.styleHelper != null) { | ||
| setFirstStyleableAncestor(node.styleHelper, styleableAncestor); |
There was a problem hiding this comment.
this changes the existing helper's ancestor unconditionally. the helper might contain stale styles (especially during style or hierarchy updates from within the listeners)
Another much better try to fix the issue.
I recommend to read: #2201 first. All tests from there are included.
I added some new ones that succeed before and after, a first step for more CSS tests as discussed in: #2218 (comment)
My new idea is now the following constraint, which I think is also a much better approach:
CssStyleHelperalways has a correctfirstStyleableAncestor. We can at any time trust and rely on it.isUserSetFontcheck to improve the performance a bitImplementation:
CssHelperStateis introduced onNode. We need to know whether we can trust thestyleHelper.Nodehas astyleHelper- it is only created when needed, so we can not attach the flag in thereThis fixes the issue while a deep (optionally unstyled) scene graph has no performance penality.
The approach is similar than my previous PR, but more smart. And with the set constraint mentioned above.
I do think we can improve the
CssStyleHelpermore. But for another day.Maybe at one point, with more tests and when all requirements are clear, we can find a way without
CssHelperStateand without creating an emptyCssStyleHelperjust to hold trigger states (because of that, we need to checkstyleHelper.cacheContainer != nulla lot of times)./issue add JDK-8187955
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2225/head:pull/2225$ git checkout pull/2225Update a local copy of the PR:
$ git checkout pull/2225$ git pull https://git.openjdk.org/jfx.git pull/2225/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2225View PR using the GUI difftool:
$ git pr show -t 2225Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2225.diff
Using Webrev
Link to Webrev Comment