On Thu, 21 May 2026 18:25:08 GMT, Andy Goryachev <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> add doc links
>
> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
> 1022:
>
>> 1020: </tr>
>> 1021: <tr>
>> 1022: <td class="value
>> nowrap">-fx-prefers-persistent-scrollbars</td>
>
> just noticed this: why the `-fx` prefix is here but not in all the others?
In CSS, this is called a vendor prefix. It is used for all symbols that have
not yet achieved what amounts to universal concensus. JavaFX uses vendor
prefixes quite extensively; in fact, almost all styleable properties are
vendor-prefixed (though not all: `visibility` and `transition` are
standard-compliant and don't use a vendor prefix).
Most of the media features we support are standard-compliant, and you'll find
that they also work exactly the same in other CSS applications like web
browsers. However, `-fx-prefers-persistent-scrollbars` is not a standard
feature; neither is `-fx-supports-conditional-feature`.
> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
> 1035:
>
>> 1033: <tr>
>> 1034: <td class="value
>> nowrap">-fx-supports-conditional-feature</td>
>> 1035: <td class="value">
>
> this cell looks way too busy. do you think it might be worth either
> extracting it into a dedicated table where each feature can be
> explained/described?
I've tried arranging the constants in several different ways, including a
table-like vertical listing. However, that introduces huge amounts of
whitespace that doesn't help readability either. I'm not too excited about
adding descriptions to all of the constants; that information is only a click
away now that we're linking to the `Platform.isSupported(ConditionalFeature)`
method. I'd like to avoid duplicating normative specification (the CSS
reference is a normative specification, not merely an informational document).
> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
> 1044:
>
>> 1042: <span class="nowrap">input-pointer</span></td>
>> 1043: <td>discrete</td>
>> 1044: <td>Evaluates to <code>true</code> if
>> <code>Platform.isSupported(ConditionalFeature)</code> returns
>
> would it be better if this were a link to
> `Platform.isSupported(ConditionalFeature)` ?
Good idea, I've added the link.
> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
> 1046:
>
>> 1044: <td>Evaluates to <code>true</code> if
>> <code>Platform.isSupported(ConditionalFeature)</code> returns
>> 1045: <code>true</code> for the specified conditional
>> feature.<br>This media feature cannot be
>> 1046: evaluated in a boolean context.</td>
>
> Could you please explain in more easily understandable terms that the
> "boolean context" means? Assume the reader is trying to use it for the first
> time.
>
> Especially confusing because `isSupported` is a boolean...
I've added a link to the paragraph "Evaluating Media Features in a Boolean
Context" which appears a bit earlier in the document.
> modules/javafx.graphics/src/test/java/test/javafx/css/CssParser_mediaQuery_Test.java
> line 385:
>
>> 383: """;
>> 384:
>> 385: Stylesheet stylesheet = new
>> CssParserShim().parseUnmerged(stylesheetText, true);
>
> in the case of invalid conditional feature like
>
> @media (-fx-supports-conditional-feature: xxx)
> {
> .button { -fx-background-color: red; }
> }
>
> the CssParser writes to stderr - is that expected?
> if so, should it be tested?
In general, I think that `CssParser` is one of the... let's call it least
well-developed parts of JavaFX. Errors from `MediaQueryParser` are fed back
into `CssParser`, where they are treated like all other errors. So that's not
"new" code at work here.
Should the way `CssParser` reports errors be tested? Maybe, but then again,
that time is better spent just discarding it entirely and replacing it with an
implementation that's not as bad. In any case, it's out of scope for this PR
because the changes here don't touch error reporting at all.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2161#discussion_r3283954890
PR Review Comment: https://git.openjdk.org/jfx/pull/2161#discussion_r3283955178
PR Review Comment: https://git.openjdk.org/jfx/pull/2161#discussion_r3283955460
PR Review Comment: https://git.openjdk.org/jfx/pull/2161#discussion_r3283956007
PR Review Comment: https://git.openjdk.org/jfx/pull/2161#discussion_r3283955671
