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 This was a hell of a ride. I updated the description with everything you need to know, what I found out and what changed overall. Everything is backed by tests, I mostly wrote the tests first to confirm the issue before writing a fix. @mstr2 If you have time, a review would be appreciated. As you also did some changes in the very same area to support transitions (especially regarding css reset + the more compliant css spec impl). ------------- PR Comment: https://git.openjdk.org/jfx/pull/2225#issuecomment-5981605221
