[External] : Re: CSS Lookups and their origins (possible regression)
John Hendrikx
john.hendrikx at gmail.com
Wed Jul 10 13:01:22 UTC 2024
I'll file a bug for this, as I feel partly responsible for uncovering
this behavior by fixing the other bug :)
I also did a deeper dive into the code already, and the comment in the
resolve lookup code seems to be misleading. It claims that it must know
the origin of the lookup so it doesn't accidentally get cached if it is
INLINE (and potentially shared with other nodes that do not have a style
defined with setStyle or a different one). Yet, no effort is made in
the calling code to exclude INLINE styles from the cache.
As it turns out, a different sub-cache is used for Nodes with inline
styles, which effectively separates the caches for nodes with certain
inline styles and nodes with different or no inline styles.
This means that most likely the style origin determination in
resolveLookups can simply be removed, and also means that the fix can be
kept limited in scope.
--John
On 09/07/2024 22:16, Kevin Rushforth wrote:
> My reading of this is that the fix for the caching bug, JDK-8245919
> [1] via PR #1072 [2], has inadvertently exposed previously hidden
> behavior that is at best an undesirable feature and at worst a bug
> (I'd call it a bug). It seems quite unexpected to me that overriding a
> variable defined in a user agent stylesheet in an AUTHOR stylesheet
> would elevate all properties derived from that variable to AUTHOR
> stylesheet status.
>
> Assuming I'm not missing something, we ought to consider fixing this.
>
> -- Kevin
>
> [1] https://bugs.openjdk.org/browse/JDK-8245919
> [2] https://github.com/openjdk/jfx/pull/1072
>
>
> On 7/9/2024 11:21 AM, John Hendrikx wrote:
>>
>> Hi Andy,
>>
>> I'm confused, nowhere do I propose to remove or otherwise make the
>> CSS reference system implemented by FX unusable.
>>
>> I'm first trying to ascertain if this would be expected behavior (it
>> is indeed unspecified, and the default currently seems to have been
>> chosen for implementation ease, not for user friendliness).
>>
>> **IF** we're considering this worth changing, the change would simply
>> be that when you override a variable (like -fx-base) that this is
>> done WITHOUT elevating all styles that use it to the level of an
>> AUTHOR stylesheet (ie. they remain at USER_AGENT level as they're
>> specified by Modena). This is not a bad view, because in a sense,
>> we're not really specifying a style, we're only overriding a
>> variable. The actual style is still specified in Modena, which is a
>> USER_AGENT level stylesheet.
>>
>> As for the bug fix, please read up a bit more on what was fixed, and
>> what this is now exposing. The fix is almost completely unrelated
>> (it fixed accidental changes to unrelated controls at the same level
>> (ie. siblings) due to cache sharing where one has had a programmatic
>> change, and the other didn't). This was caused by a CSS calculation
>> bug (calculation was skipped for all styleable properties that
>> already had a setter change, if they were encountered first by the
>> CSS system). Now that this isn't the case anymore, set values are
>> overwritten with CSS styles more aggressively. Normally those however
>> are only styles that originate from an AUTHOR stylesheet, so this can
>> be seen as expected by the user (after all, they WROTE that
>> stylesheet). But because all styles that use a variable are being
>> promoted to AUTHOR level, this also includes all unseen styles in
>> Modena if you specify the variable in your AUTHOR stylesheet.
>>
>> > Nowhere did we **actualy** override -fx-text-fill " is not
>> technically correct since this color depends on -fx-base.
>>
>> That really depends on your view point. Is overriding a variable the
>> same as defining all styles (in your AUTHOR stylesheet) that use that
>> variable? If it was a pre-processor, that created a fully resolved
>> Modena.css, then this would not be the case. But it is not
>> implemented as such.
>>
>> > And I would not want to change how it works currently because this
>> is the only way (short of overwriting the whole modena.css
>> styleshseet) for an application to effect a system-wide change like
>> reacting to changes in the user preferences or the platform theme.
>>
>> To be clear, I'm not proposing to change that at all.
>>
>> --John
>>
>> On 09/07/2024 20:00, Andy Goryachev wrote:
>>>
>>> 1) a buggy implementation coupled with lack of specification creates
>>> a certain expectation
>>>
>>> 2) bug gets fixed
>>>
>>> 3) people complain because the feature now works as it should?
>>>
>>> I think (and this is my personal opinion, in the absence of a formal
>>> specification) that this works as expected now. The statement "
>>> Nowhere did we **actualy** override -fx-text-fill" is not
>>> technically correct since this color depends on -fx-base.
>>>
>>> And I would not want to change how it works currently because this
>>> is the only way (short of overwriting the whole modena.css
>>> styleshseet) for an application to effect a system-wide change like
>>> reacting to changes in the user preferences or the platform theme.
>>>
>>> -andy
>>>
>>> *From: *John Hendrikx <john.hendrikx at gmail.com>
>>> *Date: *Tuesday, July 9, 2024 at 10:45
>>> *To: *Andy Goryachev <andy.goryachev at oracle.com>, openjfx-dev
>>> <openjfx-dev at openjdk.org>
>>> *Subject: *Re: [External] : Re: CSS Lookups and their origins
>>> (possible regression)
>>>
>>> Well, it is coming as a surprise to many. With the fix for the CSS
>>> caching bug since JavaFX 21, this "normal" behavior is becoming much
>>> more obvious.
>>>
>>> Let me repeat one more time:
>>>
>>> If I have a Label, and I manually set its text fill with a setter to
>>> YELLOW. In JavaFX 17, when I now add a stylesheet that is empty
>>> aside from `-fx-base: WHITE`, the label's text fill stays YELLOW.
>>>
>>> Now do this in JavaFX 21. As soon as you add the stylesheet with
>>> `-fx-base: WHITE` in it, the set value to YELLOW is overridden, even
>>> though technically this value for -fx-text-fill is defined by Modena
>>> (which should not be overriding set values). Nowhere did we
>>> **actualy** override -fx-text-fill, yet the CSS subsystem now sees
>>> **all** values defined by Modena that are somehow linked to -fx-base
>>> as defined directly by the developer...
>>>
>>> The reason this didn't happen in JavaFX prior to 21 is because there
>>> was a bug where a CSS value was not fully calculated if the property
>>> it encountered was overridden via a set value. That was a bug
>>> however as cache entries are shared amongst similar styled nodes,
>>> and so not calculating it fully could have effects on other nodes
>>> that shared that cache entry but did NOT have a property set
>>> directly. Now that this bug is fixed, this problem is odd behavior
>>> is popping up where simply specifying -fx-base in an empty
>>> stylesheet is somehow overriding a programmatically set text fill.
>>> Users are confused by this, as nowhere in their stylesheet do they
>>> themselves override text fill.
>>>
>>> This entire mechanism is not specified by CSS, but is unique to FX.
>>> The most similar mechanism in CSS (see Michael's answer) says the
>>> priority of a style should not be changed when it is using a reference.
>>>
>>> --John
>>>
>>> On 09/07/2024 17:43, Andy Goryachev wrote:
>>>
>>> > all styles used in Modena that rely on -fx-base directly or
>>> indirectly suddenly have a higher priority
>>>
>>> I think it works as designed (and as expected).
>>>
>>> -andy
>>>
>>> *From: *John Hendrikx <john.hendrikx at gmail.com>
>>> <mailto:john.hendrikx at gmail.com>
>>> *Date: *Tuesday, July 9, 2024 at 08:25
>>> *To: *Andy Goryachev <andy.goryachev at oracle.com>
>>> <mailto:andy.goryachev at oracle.com>, openjfx-dev
>>> <openjfx-dev at openjdk.org> <mailto:openjfx-dev at openjdk.org>
>>> *Subject: *[External] : Re: CSS Lookups and their origins
>>> (possible regression)
>>>
>>> It's not that you can't use -fx-base, but that as it is
>>> currently that all styles used in Modena that rely on -fx-base
>>> directly or indirectly suddenly have a higher priority (above
>>> setters) even though you didn't specifically specify them in
>>> your own stylesheet. All such styles are being elevated from
>>> USER_AGENT to AUTHOR level (which is above USER level which is
>>> used for setters).
>>>
>>> --John
>>>
>>> On 09/07/2024 17:03, Andy Goryachev wrote:
>>>
>>> I've used this feature in the past to change the colors in
>>> all the controls, so to me this is the expected behavior.
>>>
>>> So in your case (if I got it right), you need to set the
>>> direct style on the label
>>> (.setStyle("-fx-text-fill:yellow")) instead of setting the
>>> text fill programmatically. Right?
>>>
>>> -andy
>>>
>>> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org>
>>> <mailto:openjfx-dev-retn at openjdk.org> on behalf of John
>>> Hendrikx <john.hendrikx at gmail.com>
>>> <mailto:john.hendrikx at gmail.com>
>>> *Date: *Monday, July 8, 2024 at 17:11
>>> *To: *openjfx-dev <openjfx-dev at openjdk.org>
>>> <mailto:openjfx-dev at openjdk.org>
>>> *Subject: *Re: CSS Lookups and their origins (possible
>>> regression)
>>>
>>> I realized I worded the TLDR poorly.
>>>
>>> Let me try again:
>>>
>>> TLDR; should styles which use references (like -fx-base used
>>> in Modena) become AUTHOR level styles if -fx-base is
>>> specified in an AUTHOR stylesheet? The act of simply
>>> specifying -fx-base in your own AUTHOR stylesheet elevates
>>> hundreds of styles from Modena to AUTHOR level, as if you
>>> specified them directly...
>>>
>>> --John
>>>
>>> On 09/07/2024 02:07, John Hendrikx wrote:
>>>
>>> Hi List,
>>>
>>> TLDR; should a CSS reference like -fx-base convert all
>>> styles that use this value (or derive from it) become
>>> AUTHOR level styles (higher priority than setters) ?
>>>
>>> Long version:
>>>
>>> In JavaFX 21, I did a fix (see #1072) to solve a problem
>>> where a CSS value could be reset on an unrelated control.
>>>
>>> This happened when the CSS engine encountered a stylable
>>> that is overridden by the user (with a setter), and
>>> decided NOT to proceed with the full CSS value
>>> calculation (as it could not override the user setting
>>> if that CSS value had lower priority). However, not
>>> proceeding with the calculation meant that a "SKIP" was
>>> stored in a shared cache which was incorrect. This is
>>> because when this "SKIP" is later encountered for an
>>> unrelated control (the cache entries are shared for
>>> controls with the same styles at the same level), they
>>> could get their values reset because they were assumed
>>> to be unstyled.
>>>
>>> However, this fix has exposed what seems to be a deeper
>>> bug or perhaps an unfortunate default:
>>>
>>> JavaFX has a special feature where you can refer to
>>> certain other styles by using a reference (which is
>>> resolved, recursively, to a final value). This does not
>>> seem to be a CSS standard, but is a feature only FX has.
>>>
>>> It works by saying something like:
>>>
>>> -fx-base: RED;
>>>
>>> And then using it like this:
>>>
>>> -fx-text-fill: -fx-base;
>>>
>>> This feature works accross stylesheets of different
>>> origins, so an AUTHOR stylesheet can specify -fx-base,
>>> and when a USER_AGENT refers to -fx-base, the value
>>> comes from the AUTHOR stylesheet.
>>>
>>> JavaFX then changes the origin of the style to the
>>> highest priority encountered while resolving the
>>> reference. This means that Modena can specify
>>> "-fx-text-fill: -fx-base", and when "-fx-base" is then
>>> part of the AUTHOR style sheet, that ALL Modena styles
>>> that use -fx-base will be considered AUTHOR level
>>> styles, as per this comment:
>>>
>>> // The origin of this parsed value is the greatest of
>>>
>>> // any of the resolved reference. If a resolved reference
>>>
>>> // comes from an inline style, for example, then the value
>>>
>>> // calculated from the resolved lookup should have inline
>>>
>>> // as its origin. Otherwise, an inline style could be
>>>
>>> // stored in shared cache.
>>>
>>> I feel that this is a really unfortunate choice. The
>>> style after all was specified by Modena, only its value
>>> came from another (higher priority) style sheet. I
>>> think a more logical choice would have been to not
>>> change the priority at all, unless a "-fx-text-fill" is
>>> explicitly made part of the AUTHOR stylesheet.
>>>
>>> A consequence of this (and which is much more visible
>>> after the fix) is that creating a Label with a
>>> setTextFill(Color.YELLOW) in its constructor will only
>>> result in a yellow text fill if the AUTHOR stylesheet
>>> did not override any of the Modena colors involved in
>>> calculating the Modena -fx-text-fill default.
>>> Overriding -fx-base in any way will result in the text
>>> fill of the label to be overridden (as the reference
>>> came from an AUTHOR stylesheet, which trumps a setter
>>> which is of a lower style origin).
>>>
>>> The comment also alludes to a potential problem. If an
>>> inline style would specify "-fx-base", but would be
>>> treated as if it came from Modena (USER_AGENT level),
>>> then this value could get stored in the cache as
>>> everything except INLINE styles can be cached. However,
>>> I feel that the changing of style origin level was then
>>> probably done to solve a CACHING problem, instead of
>>> what made logical sense for users. If we agree that a
>>> resolved reference should not change the style origin
>>> level, then this would need to be addressed, by perhaps
>>> marking such a calculated value as uncacheable, instead
>>> of overloading the meaning of style origin.
>>>
>>> I'd like to hear your thoughts, and also how to
>>> proceed. JavaFX versions before 21 seemingly allowed
>>> overriding reference without much consequence because if
>>> the user overrode the value manually, the cache entry
>>> would be set to "SKIP". Now that this is no longer the
>>> case, JavaFX more aggressively overrides user set values
>>> if they happen to use a referenced value. See code below.
>>>
>>> --John
>>>
>>> .root {
>>>
>>> -fx-base: #ff0000;
>>>
>>> }
>>>
>>> *package*app;
>>>
>>> *import*javafx.application.Application;
>>>
>>> *import*javafx.scene.Scene;
>>>
>>> *import*javafx.scene.control.Label;
>>>
>>> *import*javafx.scene.paint.Color;
>>>
>>> *import*javafx.stage.Stage;
>>>
>>> *public**class*TestApp *extends*Application {
>>>
>>> *public**static**void*main(String[] args) {
>>>
>>> /launch/(args);
>>>
>>> }
>>>
>>> @Override
>>>
>>> *public**void*start(Stage primaryStage) {
>>>
>>> Scene scene = *new*Scene(*new*MyLabel());
>>>
>>> // See the difference with/without -fx-base in the
>>> _stylesheet_
>>>
>>> scene.getStylesheets().add(TestApp.*class*.getResource("/style.css").toExternalForm());
>>>
>>> primaryStage.setScene(scene);
>>>
>>> primaryStage.show();
>>>
>>> }
>>>
>>> }
>>>
>>> *class*MyLabel *extends*Label {
>>>
>>> *public*MyLabel() {
>>>
>>> setTextFill(Color.YELLOW);
>>>
>>> setText("Hello world");
>>>
>>> }
>>>
>>> }
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240710/0c4bf4ec/attachment-0001.htm>
More information about the openjfx-dev
mailing list