RFR: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet [v2]

[Andy Goryachev]

On Mon, 15 Jul 2024 12:10:27 GMT, John Hendrikx <jhendrikx at 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