jfx 8345348: CSS media feature queries

Implementation of CSS media queries.

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
[ ] Change requires CSR request JDK-8358332 to be approved

Issues

JDK-8345348: CSS media feature queries (Enhancement - P4)
JDK-8358332: CSS media feature queries (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 6229a69e)
John Hendrikx (@hjohn - Reviewer) 🔄 Re-review required (review applies to 6229a69e)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1655/head:pull/1655
$ git checkout pull/1655

Update a local copy of the PR:
$ git checkout pull/1655
$ git pull https://git.openjdk.org/jfx.git pull/1655/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1655

View PR using the GUI difftool:
$ git pr show -t 1655

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1655.diff

Using Webrev

Link to Webrev Comment

Dec 02 '24 16:12 mstr2

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Dec 02 '24 16:12 bridgekeeper[bot]

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8345348: CSS media feature queries

Reviewed-by: angorya, jhendrikx, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

d6265e49c3832972e8005913fbb3c126ba930577: 8361710: Mark QPathTest as unstable on all platforms
6029e3e4058e2a87ecb0f5c07689f7d4783a1c65: 8359387: Bump minimum JDK version for JavaFX to JDK 23
639a5950b6eae7870fd9d9e84f9ce81322aaab82: 8357584: [XWayland] [OL10] Robot.mousePress() is delivered to wrong place
... and 4 more: https://git.openjdk.org/jfx/compare/fc4642dbb008ed0e49996c1eea10b92fad5f7dcf...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Dec 02 '24 16:12 openjdk[bot]

/reviewers 2 reviewer /csr

Dec 02 '24 23:12 mstr2

@mstr2 The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

Dec 02 '24 23:12 openjdk[bot]

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8345348 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Dec 02 '24 23:12 openjdk[bot]

Webrevs

Dec 02 '24 23:12 mlbridge[bot]

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Dec 31 '24 01:12 bridgekeeper[bot]

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/media-queries
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Jan 16 '25 16:01 openjdk[bot]

@mstr2 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Jan 28 '25 02:01 bridgekeeper[bot]

Let's try this again.

/open

Apr 03 '25 19:04 mstr2

@mstr2 This pull request is now open

Apr 03 '25 19:04 openjdk[bot]

I think you should consider moving MediaRule, MediaQuery and MediaQueryContext to com.sun.javafx.css (or similar internal package). The javafx.css module IMHO should not be further extended in its current form, as IMHO it needs a rethink first. From what I gather it was only made public to support writing binary CSS files, which was done primarily to be able to load huge stylesheets like caspian/modena a bit faster; it's public interface was never really thought through, and there are no known users (aside from Scene Builder), and AFAIK no use cases for the API (StyleSheet can't be directly used by the CSS engine, and doing so would probably cripple it as the StyleSheet class seems to be observable in all its aspects -- I have doubts any of that is currently in use, but perhaps Scene Builder uses it).

I verified on a local build that this is easy to do (you need to make some constructors public, and the writeBinary/readBinary functions after they've been moved to the internal package).

Apr 04 '25 12:04 hjohn

In the .md document, you make a reference to w3.org media queries, so one question comes right away: do you eventually plan to implement the whole spec as defined there, or we are really talking about a very small subset of features?

For example, do you plan to support media query operations like and, not, only, etc.? Also, the w3.org spec might be way too complicated even to be referenced - things there will never apply to JavaFX (tty, projection, speech, etc.)

Alternatively, wouldn't it make more sense to add variables to the JavaFX CSS instead? Or maybe in conjunction with a simpler media-query-like syntax, something like @if(scene.isDark)?

My concern is that the media query features (whatever subset of it we decide to implement) might make the code way too complex without providing substantial benefit, and perhaps we can achieve the same goal (of dynamically activating parts of the stylesheet) using simpler solutions like variables?

What do you think?

Apr 10 '25 16:04 andy-goryachev-oracle

Also, would it be possible to add Goals and Non-Goals to the .md, make it more JEP-like?

Apr 10 '25 16:04 andy-goryachev-oracle

In the .md document, you make a reference to w3.org media queries, so one question comes right away: do you eventually plan to implement the whole spec as defined there, or we are really talking about a very small subset of features?

We'd probably never implement the whole spec, but only the parts that make sense for JavaFX. Starting with user-preference media features is an obvious choice, because we already have a matching set of platform preferences (the astute reader might notice just how closely the existing platform preferences, the proposed media queries, and the W3C standard align -- almost as if by design :wink:).

For example, do you plan to support media query operations like and, not, only, etc.? Also, the w3.org spec might be way too complicated even to be referenced - things there will never apply to JavaFX (tty, projection, speech, etc.)

The only keyword makes no sense here, it's a legacy keyword for the web. Other than that, this PR already implements the full syntax for media feature queries, including and, or, and not (except for the range form, which we only need to support if we decide to also support viewport characteristics features like width).

I reference the W3C specification because like with all recently added CSS features, this is an implementation of the standard. The JavaFX CSS reference documents the extent to which the standard is implemented.

Alternatively, wouldn't it make more sense to add variables to the JavaFX CSS instead? Or maybe in conjunction with a simpler media-query-like syntax, something like @if(scene.isDark)?

CSS variables can't be implemented without a major rewrite of the CSS system. We can do this, but it is not really related to the feature proposed here. Even with CSS variables, you need a way to toggle their value based on external configuration, and then you're right back at the start of the problem.

I am very much against inventing any non-standard CSS syntax whatsoever. Every time JavaFX did that, the result was not good. Let's face the reality here: the JavaFX CSS implementation is severely undercooked, and that's the very best thing I can say about it. I don't want to make it any worse.

My concern is that the media query features (whatever subset of it we decide to implement) might make the code way too complex without providing substantial benefit, and perhaps we can achieve the same goal (of dynamically activating parts of the stylesheet) using simpler solutions like variables?

You seem to be arguing against media queries themselves, not against their implementation in JavaFX. But they are a thing in the technology that 99,9% of developers know as CSS, it's a brute fact of nature. I think we should acknowledge that, and not try to do our own little thing again.

Apr 10 '25 16:04 mstr2

Also, would it be possible to add Goals and Non-Goals to the .md, make it more JEP-like?

I added the two sections.

Apr 10 '25 17:04 mstr2

You seem to be arguing against media queries themselves

Probably. I am afraid the CSS subsystem will grow into a monster (I mean, it already is).

The follow up question is what you guys think about updating modena.css to support the dark mode? I understand the platform preferences and these media queries are prerequisites, what are other things that we'll need?

Apr 11 '25 22:04 andy-goryachev-oracle

You seem to be arguing against media queries themselves

Probably. I am afraid the CSS subsystem will grow into a monster (I mean, it already is).

It is, and it’s held together by duct tape in many places. That’s why I think we should consider the rewrite at some point in the future to put it on solid footing (and it’s the only way to get really useful things like variables).

Media queries are pretty orthogonal to that, as most of the implementation will be the same with a potential rewrite.

The follow up question is what you guys think about updating modena.css to support the dark mode? I understand the platform preferences and these media queries are prerequisites, what are other things that we'll need?

What’s missing to properly support dark mode is really only the external configuration of stylesheets, so I think with media queries in place we have everything we need for a dark version of Modena.

Apr 12 '25 10:04 mstr2

You seem to be arguing against media queries themselves

Probably. I am afraid the CSS subsystem will grow into a monster (I mean, it already is).

The follow up question is what you guys think about updating modena.css to support the dark mode? I understand the platform preferences and these media queries are prerequisites, what are other things that we'll need?

With some minimal style changes (and a hack to change the window border to dark mode on Windows) I get this out of modena.css:

Apr 12 '25 18:04 hjohn

With some minimal style changes (and a hack to change the window border to dark mode on Windows) I get this out of modena.css:

Nice! Once we have a per-scene color scheme, I plan to have window borders adjust to the color scheme of the scene.

By the way, can I interest you in a review of this PR?

Apr 12 '25 20:04 mstr2

By the way, can I interest you in a review of this PR?

Yes, I already partially checked it out, and was intending to do a closer review soon.

Apr 12 '25 21:04 hjohn

Would it be possible to expand the examples in the .md file? For example, an example that uses operator(s)? edit: in the cssref.html, there is an example of invalid combination, but no example of a valid combination. I think it would be helpful to include an example which uses an operator.

Another question: is there any special prioritization given to media queries relative to other rules?

May 02 '25 22:05 andy-goryachev-oracle

Another question: is there any special prioritization given to media queries relative to other rules?

No, a media query just either matches, or it doesn't. If it matches, the rule enclosed by the media query is treated like any other rule; if it doesn't match, the rule doesn't apply.

May 03 '25 08:05 mstr2

if it doesn't match, the rule doesn't apply.

and if does apply, it overrides any previous rules (but can be overridden by any subsequent rules), right?

May 05 '25 18:05 andy-goryachev-oracle

and if does apply, it overrides any previous rules (but can be overridden by any subsequent rules), right?

Yes, if the media query matches, it's as if the enclosed rule was a normal rule at that position in the stylesheet.

May 05 '25 18:05 mstr2

Should this work?

@media (prefers-color-scheme: dark) and not (prefers-persistent-scrollbars) {
  .root {
    -fx-accent: yellow;
  }
}

(it does not, writes to stderr):

WARNING: CSS Error parsing '@media (prefers-color-scheme: dark) and not (prefers-persistent-scrollbars) {
  .root {
    -fx-accent: yellow;
  }
}: Expected LPAREN at [1,40]

Also, are people supposed to know what LPAREN is? Can we provide more user friendly error messages?

May 05 '25 20:05 andy-goryachev-oracle

Should this work?

No. I've added a test for it.

Also, are people supposed to know what LPAREN is? Can we provide more user friendly error messages?

The error messages of CssParser are what they are, I guess. For example, it will also emit messages like "Expected LBRACE" and "Expected RBRACE". If we change that, then probably with a cleanup ticket.

May 06 '25 06:05 mstr2

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jun 30 '25 14:06 bridgekeeper[bot]

/integrate

Jul 11 '25 16:07 mstr2

Going to push as commit e0f8e720752aecffb9090d1a3b82317b518f94a6. Since your change was applied there have been 7 commits pushed to the master branch:

d6265e49c3832972e8005913fbb3c126ba930577: 8361710: Mark QPathTest as unstable on all platforms
6029e3e4058e2a87ecb0f5c07689f7d4783a1c65: 8359387: Bump minimum JDK version for JavaFX to JDK 23
639a5950b6eae7870fd9d9e84f9ce81322aaab82: 8357584: [XWayland] [OL10] Robot.mousePress() is delivered to wrong place
... and 4 more: https://git.openjdk.org/jfx/compare/fc4642dbb008ed0e49996c1eea10b92fad5f7dcf...master

Your commit was automatically rebased without conflicts.

Jul 11 '25 16:07 openjdk[bot]

jfx jfx copied to clipboard

8345348: CSS media feature queries

Progress

Issues

Reviewers

Reviewing

Webrevs

jfx
jfx copied to clipboard