On Mon, 15 Jul 2024 12:10:27 GMT, John Hendrikx <[email protected]> wrote:
>> This change removes the origin determination from `resolveLookups`.
>> Instead, the origin from the style is used.
>>
>> Although a comment in the code alluded that this may cause problem with
>> `INLINE` styles, this is not the case. Whenever a `Node` is associated with
>> a `CssStyleHelper`, a suitable shared cache is determined for its use. This
>> already takes into account the presence of an inline style, and only nodes
>> with the same inline style can share such a cache. See `Cache#getStyleMap`
>> and specifically this fragment where an additional selector is added for the
>> inline style:
>>
>> if (hasInlineStyle) {
>> Selector selector =
>> cacheContainer.getInlineStyleSelector(inlineStyle);
>> if (selector != null) selectors.add(selector);
>> }
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add test case
The fix looks good, presuming that you are correct, and that the comment about
needing to set the origin of the parsed value to the greatest of any resolved
reference in order to avoid storing an inline style in the shared cache is in
fact wrong. Testing should help catch any potential problems.
The new unit test looks like it covers all the cases. As with PR #1505, this PR
mixes JUnit 4 and JUnit 5, which we really don't want to do in the same test
class. Consider converting the entire class to JUnit 5, perhaps in #1505.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1503#issuecomment-2231564853
