Skip to content

Install fonts-noto-cjk for linux-wpt workflow#35567

Merged
mukilan merged 1 commit intoservo:mainfrom
xiaochengh:install-noto-cjk
Feb 21, 2025
Merged

Install fonts-noto-cjk for linux-wpt workflow#35567
mukilan merged 1 commit intoservo:mainfrom
xiaochengh:install-noto-cjk

Conversation

@xiaochengh
Copy link
Copy Markdown
Contributor

@xiaochengh xiaochengh commented Feb 21, 2025

Right now CJK-related tests are not running properly as the characters are rendered as tofu.

This PR installs fonts-noto-cjk to fix it. Also:


  • There are tests for these changes OR
  • These changes do not require tests because ___

@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#50853) with upstreamable changes.

@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#50853) title and body.

@jschwe
Copy link
Copy Markdown
Member

jschwe commented Feb 21, 2025

Rebasing should fix the test-tidy failure.

@xiaochengh xiaochengh marked this pull request as ready for review February 21, 2025 06:10
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@xiaochengh
Copy link
Copy Markdown
Contributor Author

@delan @Loirooriol PTAL, thanks!

Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mrobinson mrobinson enabled auto-merge February 21, 2025 07:35
@mukilan
Copy link
Copy Markdown
Member

mukilan commented Feb 21, 2025

Looks like the DCO workflow has not started, for some reason. @xiaochengh can you try rebasing and doing a force push?

Signed-off-by: Xiaocheng Hu <[email protected]>
auto-merge was automatically disabled February 21, 2025 10:01

Head branch was pushed to by a user without write access

@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#50853).

Copy link
Copy Markdown
Member

@jschwe jschwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really okay to just remove size-adjust from the upstream wpt test?

@mrego
Copy link
Copy Markdown
Member

mrego commented Feb 21, 2025

Is it really okay to just remove size-adjust from the upstream wpt test?

This is modifying the reference file, so it doesn't use size-adjust property. The test still uses it. It's good to avoid using the same property in the reference file, as it can cause false passes like in this case.

Also with this change, the test is still passing in Chromium, Firefox and failing in Safari as before this change, see: https://github.com/web-platform-tests/wpt/pull/50853/checks?check_run_id=37595996561

@mukilan mukilan added this pull request to the merge queue Feb 21, 2025
Merged via the queue into servo:main with commit 2b0d2ec Feb 21, 2025
22 checks passed
@Loirooriol
Copy link
Copy Markdown
Contributor

No big deal, but I don't see the need to change the ref, it seems to make the test slightly more fragile in case that size-adjust: 1000% and font-size: 1000% were to behave slightly different on some browser/platform (not much familiar with size-adjust to know if it's possible). Not supporting a feature doesn't mean that absolutely all tests related to it must fail.

@xiaochengh
Copy link
Copy Markdown
Contributor Author

xiaochengh commented Feb 21, 2025

@Loirooriol size-adjust: 1000% is specified to work the same as font-size: 1000%. I wrote the spec :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

7 participants