On Mon, 15 Jul 2024 12:10:27 GMT, John Hendrikx <jhendr...@openjdk.org> 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
Thank you for all the explanations!
I did a quick test with the Monkey Tester, setting various styles. Haven't
found any issues.
Left a few comments inline.
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 73:
> 71: }
> 72:
> 73: @BeforeEach
since we are now creating Stage every time, shouldn't we `hide()` it each time?
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 696:
> 694: AUTHOR_OVERRIDES_USER_AGENT(RED_STYLESHEET, null,
> GRAY_STYLESHEET, null),
> 695: AUTHOR_OVERRIDES_INDIRECT_USER_AGENT(RED_INDIRECT_STYLESHEET,
> null, GRAY_STYLESHEET, null),
> 696: AUTHOR_OVERRIDES_PROPERTY(RED_STYLESHEET, Color.BLUE,
> GRAY_STYLESHEET, null),
should another one be added here:
`AUTHOR_OVERRIDES_PROPERTY2(RED_INDIRECT_STYLESHEET, Color.BLUE,
GRAY_STYLESHEET, null),`
and also
`AUTHOR_OVERRIDES_PROPERTY(RED_INDIRECT_STYLESHEET, Color.BLUE,
GRAY_STYLESHEET, "-fx-base: yellow"),`
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 705:
> 703: INLINE_OVERRIDES_USER_AGENT(RED_STYLESHEET, null, null,
> "-fx-background-color: #808080"),
> 704: INLINE_OVERRIDES_PROPERTY(RED_STYLESHEET, Color.BLUE, null,
> "-fx-background-color: #808080"),
> 705: INLINE_OVERRIDES_AUTHOR(RED_STYLESHEET, null, RED_STYLESHEET,
> "-fx-background-color: #808080"),
Should we add other combinations:
(R,B,R,..
(RED_INDIRECT,..
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 710:
> 708:
> INLINE_VARIABLE_OVERRIDES_USER_AGENT_VARIABLE(RED_INDIRECT_STYLESHEET, null,
> null, "-fx-base: #808080"),
> 709:
> INLINE_VARIABLE_OVERRIDES_AUTHOR_VARIABLE(RED_INDIRECT_STYLESHEET, null,
> FX_BASE_GREEN_STYLESHEET, "-fx-base: #808080");
> 710:
also, would it make sense to add tests for null/empty userAgentStylesheet, for
completeness sake?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1503#pullrequestreview-2189475186
PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1685117648
PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1685082283
PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1685085547
PR Review Comment: https://git.openjdk.org/jfx/pull/1503#discussion_r1685086065
