On Sun, 4 Oct 2026 14:18:33 GMT, Marius Hanl <[email protected]> wrote:
>> Another much better try to fix the issue. >> I recommend to read: https://github.com/openjdk/jfx/pull/2201 first. All >> tests from there are included. >> I added some new ones that succeed before and after, a good step forward for >> more CSS tests as discussed in: >> https://github.com/openjdk/jfx/pull/2218#issuecomment-5094495306. >> >> My new idea is now the following constraint, which I think is also a much >> better approach: >> - A `CssStyleHelper` always has a correct `firstStyleableAncestor`. We can >> at any time trust and rely on it >> - Like the `CacheContainer` >> - We will build and reuse a 'styleable chain' when creating the >> `CssStyleHelper` >> - Very rarely, we are rebuilding a child first while an ancestor's >> `CssStyleHelper` is stale. In this situation, we will rebuild the ancestor >> and save a flag. Since we are reusing the chain, we will not do more work >> than needed in any case >> >> ### Problem >> >> The last days, I invested much time in all possible scenarios that may break >> the assumptions above, that is, the scene structure changes while we are >> currently creating our `CssStyleHelper`. And there are many such situations. >> I first tried fixing all of them, but the logic at one point got very >> complex and what I really did not like: We need to detect and start over >> when the creation of a `CssStyleHelper` changed the node structure. >> >> ### Fix >> >> Why does this happen? Because the creation of `CssStyleHelper` may reset CSS >> properties, which will run listeners that could change the node structure or >> node styles. >> >> To fix all the issues, the solution is actually simple and preexisting: >> `transitionToState` will reset the css properties. >> >> `transitionToState` already handled all cases where css properties must be >> reset except one: When the property disappeared from the new style map >> entirely (e.g. due to a changed style class). This is now changed. >> As a bonus, this makes it actually more CSS spec compliant for transitions! >> See: https://www.w3.org/TR/css-transitions-1/#starting, quoting: >> >> >> Note that the above rules mean that when the computed value of an animatable >> property changes, the transitions that start are based on the values of the >> ... transition-* ... properties at the time the animatable property would >> first have its new computed value. >> This means that when one of these transition-* properties changes at the >> same time as a property whose change might transition, it is the new values >> of the transition-* properties that control t... > > Marius Hanl has updated the pull request incrementally with three additional > commits since the last revision: > > - Simplify the code a little bit > - New approach: Reset cssProperties on transitionToState. > > This fixes basically all weird cases we could have where listeners run > during StyleHelper creation to break all our assumptions. > > transitionToState already handled all cases where css properties must be > reset except one: When the property disappeared from the new style map > entirely (e.g. changed style class). This is now changed. This makes it > actually more CSS spec compliant for transitions and in general, improves the > behavior by letting one method do, well the transition. > - corner case https://github.com/openjdk/jfx/pull/2333 definitely helped, but still some regressions. modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 227: > 225: // The css set properties carry over, so those no longer styled > are reset when the styles are applied. > 226: if (currentHelper != null) { > 227: > helper.cacheContainer.cssSetProperties.putAll(currentHelper.cacheContainer.cssSetProperties); we'll have a regression: here we copy old properties (even those removed), mixing them with the new. example: 1. style translateX in the stylesheet (.old), style opacity in .new (removing translateX) 2. applyCss() 3. add a listener to translateX which sets opacity 4. set style to .new on the root, applyCss() modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 318: > 316: } > 317: > 318: final CssStyleHelper helper = node.styleHelper; don't really need `final` for effectively final vars modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 899: > 897: // CssMetaData#getStyleableProperty which is rather > expensive as it may cause expansion of lazy > 898: // properties. > 899: CalculatedValue initialValue = > cacheContainer.cssSetProperties.get(cssMetaData); another regression: when the function returns SKIP, it's read with get() but never removed from hte cssSetProperties. if the property is set by user, styled, then unstyled, the new USER value is not captured LL960-4 because the stale entry still exists. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10000: > 9998: cssFlag = CssFlags.REAPPLY; > 9999: > 10000: final boolean updateChildren = > CssStyleHelper.createStyleHelper(this); `final` ------------- Changes requested by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2225#pullrequestreview-5419739126 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4188639231 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4188082586 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4188774269 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4188095815
