Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-14749] Errors in styles.css #8148

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

basil
Copy link
Member

@basil basil commented Jun 14, 2023

This PR adds a test that fails without the change to src/main and passes with it. The test opens various common pages in HtmlUnit and verifies that there are no CSS errors (except for YUI CSS errors, which we ignore both here and in the test harness). This will prevent any new CSS that HtmlUnit cannot understand from being introduced in the future. It will also ensure that the logs stay clean going forward. To fix the existing violation, I removed the use of :has (which HtmlUnit does not understand) in favor of a simpler CSS construct.

Alternative approach that I considered

I tried setting up PostCSS Preset Env with the has-pseudo-class polyfill to allow us to continue using this modern CSS while appeasing HtmlUnit. However the polyfill has two parts: a CSS part and a JavaScript part, and HtmlUnit choked on the JavaScript part of the polyfill. So I found it simpler to eliminate the use of :has instead.

Testing done

New unit test fails without the changes to src/main and passes with them.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@basil basil added the skip-changelog Should not be shown in the changelog label Jun 14, 2023
@basil basil requested a review from janfaracik June 14, 2023 21:10
@basil
Copy link
Member Author

basil commented Jun 15, 2023

Note that the "Alternative approach that I considered" described above in the PR description did work for transpiling media feature range notation from newer CSS to HtmlUnit-compatible older CSS, even though that approach did not work for the :has pseudo class due to the polyfill requiring both CSS and (HtmlUnit-incompatible) JavaScript. In general, I think we should be able to transpile CSS down to an HtmlUnit-compatible version in most cases using the approach in #8149.

@NotMyFault NotMyFault requested a review from a team June 21, 2023 10:42
@NotMyFault
Copy link
Member

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 26, 2023
@NotMyFault NotMyFault merged commit 0ae6ad5 into jenkinsci:master Jun 27, 2023
NotMyFault added a commit to NotMyFault/jenkins that referenced this pull request Jun 28, 2023
yaroslavafenkin pushed a commit to yaroslavafenkin/jenkins that referenced this pull request Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants