On Thu, 23 Jul 2026 19:18:48 GMT, Andy Goryachev <[email protected]> wrote:
>> You are right that `41.0` would make more sense here, but that's assuming we >> could get that value at all. The source of the exception used here is just >> to test the `SKIP` adding code path (as it is crucial now that we add `SKIP` >> or a value for every value in the CSS metadata). The path I was testing >> however is *normally* only triggered when `lookup` fails, and when that >> fails there simply isn't a value to put in the cache at all (ie. wrong type >> in the CSS or you used a reference that doesn't exist). `applyStyle` isn't >> allowed to throw exception, so results are "best effort" (and how this >> worked is pre-existing, I didn't change it). > > My understanding is that if one control fails to applyStyle, the other > control (which encountered no issues) should pick up the values from the > stylesheet, and certainly not the default. > > Or am I missing something? I must point out: `applyStyle` is not allowed to fail, so this is pretty much an edge case already (which I just exploit in the test to ensure the exceptional path is adding `SKIP` in exceptional cases). The exceptions are supposed to come from `lookup`, which if it fails with an exception means that you simply do not know what the CSS value could be. This is then stored in the cache entry, and any sharing of that entry assumes the entry is created correctly (which it was if `lookup` was failing, as there is no retrying here as the CSS evaluation will just return the same exception). So that it resets the value for other Nodes is expected, as there is no CSS value known as computation failed (artificially in this case, but failed nonetheless). Again, in this specific case, I agree we could do better, but it seems beyond the scope of this fix (it has always worked like this -- an exception in `applyStyle` resulted in the system assuming that for that cache entry there is no CSS based value -- no matter how wrong that assumption is). I can fix this as well if you want, but: - `applyStyle` is not allowed to throw exceptions, so what are we truly fixing here? - I didn't change this - It has nothing to do with the problem that triggered the creation of this fix (which IMHO is already not something I caused... so I'm being nice here). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3641874177
