layout: Resolve word-spacing percentages against computed font-size#44031
Merged
Conversation
servo-highfive
added
the
S-awaiting-review
Member
|
@sabbCodes The testing field refers to automated testing and is not meant as place to show screenshots of behavior changes. Are you able to write an automated test that shows this fix? |
|
🔨 Triggering try run (#24142085495) for Linux (WPT) |
Member
|
We should also link to the specification discussion for this issue and give some background about why we are making these changes in the PR description. |
Contributor
|
Should be covered by |
Loirooriol
reviewed
Apr 8, 2026
Comment on lines
+411
to
+413
| inherited_text_style | ||
| .word_spacing | ||
| .to_used_value(Au::from_f64_px(space_width)) | ||
| .to_used_value(parent_style.clone_font().font_size.computed_size().into()) |
Contributor
There was a problem hiding this comment.
This doesn't depend on segment. You can resolve above in the word_spacing variable before the loop.
Contributor
Author
Yes, I found one here This was the result; I wasn't sure what to do, so I followed only the testcase described in #43997 |
Contributor
|
Yes, it's expected to fail because currently it fails. After your fix, the expectation should be to pass. See https://book.servo.org/contributing/testing.html#updating-web-platform-test-expectations |
|
Test results for linux-wpt from try job (#24142085495): Flaky unexpected result (37)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (1)
|
|
|
Contributor
Author
|
Hi @Loirooriol Here's the new result to |
Loirooriol
reviewed
Apr 8, 2026
| let word_spacing = inherited_text_style.word_spacing.to_length().map(Au::from); | ||
| let word_spacing = inherited_text_style | ||
| .word_spacing | ||
| .to_length() |
Contributor
There was a problem hiding this comment.
You can just use to_used_value() directly
Loirooriol
reviewed
Apr 8, 2026
| .unwrap_or_else(|| { | ||
| inherited_text_style | ||
| .word_spacing | ||
| .to_used_value(parent_style.clone_font().font_size.computed_size().into()) |
Contributor
There was a problem hiding this comment.
You can share parent_style.clone_font().font_size.computed_size() with letter_spacing.
And in fact, better parent_style.get_font().font_size.computed_size() to avoid cloning the entire font.
Loirooriol
reviewed
Apr 8, 2026
| flags.insert(ShapingFlags::DISABLE_KERNING_SHAPING_FLAG) | ||
| } | ||
| let word_spacing = inherited_text_style.word_spacing.to_length().map(Au::from); | ||
| let word_spacing = inherited_text_style |
Contributor
There was a problem hiding this comment.
While you are at it, I would move let word_spacing to be next to let letter_spacing
Contributor
Author
There was a problem hiding this comment.
You want me to move them together?
Contributor
Author
There was a problem hiding this comment.
Like so;
let letter_spacing = inherited_text_style.letter_spacing.0.resolve(font_size);
let word_spacing = inherited_text_style
.word_spacing
.to_used_value(font_size.into());
?
Contributor
There was a problem hiding this comment.
Well, after the logic to turn a zero letter_spacing into None, I guess. Or put word_spacing first.
Also, do you need the .into()?
Contributor
Author
There was a problem hiding this comment.
Yeah, compiler complains in its absence
.to_used_value(font_size);
| ------------- ^^^^^^^^^ expected `Au`, found `CSSPixelLength`
| |
| arguments to this method are incorrect
|
note: method defined here
--> /home/sabb/.cargo/git/checkouts/stylo-482338307e42a9ea/ddf2109/style/values/computed/length_percentage.rs:531:12
|
531 | pub fn to_used_value(&self, containing_length: Au) -> Au {
| ^^^^^^^^^^^^^
help: call `Into::into` on this expression to convert `CSSPixelLength` into `Au`
|
380 | .to_used_value(font_size.into());
| +++++++
``
servo-highfive
added
S-needs-code-changes
Loirooriol
changed the title
word-spacing percentages against computed font-size
Contributor
Author
Right |
Loirooriol
approved these changes
Apr 8, 2026
| .to_used_value(font_size.into()); | ||
| let letter_spacing = inherited_text_style.letter_spacing.0.resolve(font_size); | ||
| let letter_spacing = if letter_spacing.px() != 0. { | ||
| Some(app_units::Au::from(letter_spacing)) |
Contributor
There was a problem hiding this comment.
Nit: it may look more consistent like
let font_size = parent_style.get_font().font_size.computed_size().into();
let word_spacing = inherited_text_style.word_spacing.to_used_value(font_size);
let letter_spacing = inherited_text_style.letter_spacing.0.to_used_value(font_size);
let letter_spacing = if !letter_spacing.is_zero() {
Some(letter_spacing)
Contributor
Author
There was a problem hiding this comment.
It''l fail the ./mach test-tidy test if we do it like that, the ./mach fmt command wants it as is
Contributor
There was a problem hiding this comment.
I meant using to_used_value() for both. But run ./mach fmt, sure
Contributor
Author
There was a problem hiding this comment.
Ohh, lemme try it.
Hope it builds successfully
Contributor
Author
There was a problem hiding this comment.
So, using to_used_value() for letter_spacing introduces the following error;
[{
"resource": "/home/sabb/servo/components/layout/flow/inline/text_run.rs",
"owner": "rust-analyzer",
"code": {
"value": "E0308",
"target": {
"$mid": 1,
"path": "/stable/error_codes/E0308.html",
"scheme": "https",
"authority": "doc.rust-lang.org"
}
},
"severity": 8,
"message": "expected Au, found CSSPixelLength",
"source": "rust-analyzer",
"startLineNumber": 381,
"startColumn": 82,
"endLineNumber": 381,
"endColumn": 91,
"origin": "extHost1"
}]
[{
"resource": "/home/sabb/servo/components/layout/flow/inline/text_run.rs",
"owner": "rustc",
"code": {
"value": "Click for full compiler diagnostic",
"target": {
"$mid": 1,
"path": "/diagnostic message [3]",
"scheme": "rust-analyzer-diagnostics-view",
"query": "3",
"fragment": "file:///home/sabb/servo/components/layout/flow/inline/text_run.rs"
}
},
"severity": 8,
"message": "no method named `px` found for struct `app_units::Au` in the current scope\nmethod not found in `app_units::Au`",
"source": "rustc",
"startLineNumber": 382,
"startColumn": 48,
"endLineNumber": 382,
"endColumn": 50,
"origin": "extHost1"
}]
Contributor
There was a problem hiding this comment.
Use into() so that font_size is an Au
Contributor
Author
There was a problem hiding this comment.
I did, there's a different error, I think that one was about the .into() not existing on Au
Lemme check again
Contributor
Author
There was a problem hiding this comment.
Okay, this is weird, lol
I think it wass my machine that was still rendering the previous error after I added the .into() yesterday, cause now it's stopped complaining.
But there's still this;
[{
"resource": "/home/sabb/servo/components/layout/flow/inline/text_run.rs",
"owner": "rustc",
"code": {
"value": "Click for full compiler diagnostic",
"target": {
"$mid": 1,
"path": "/diagnostic message [0]",
"scheme": "rust-analyzer-diagnostics-view",
"query": "0",
"fragment": "file:///home/sabb/servo/components/layout/flow/inline/text_run.rs"
}
},
"severity": 8,
"message": "no method named `px` found for struct `app_units::Au` in the current scope\nmethod not found in `app_units::Au`",
"source": "rustc",
"startLineNumber": 382,
"startColumn": 48,
"endLineNumber": 382,
"endColumn": 50,
"origin": "extHost1"
}]
Contributor
There was a problem hiding this comment.
Check my code. Drop px()
servo-highfive
added
S-awaiting-review
Contributor
|
You will also need to rebase after #43974 |
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Loirooriol
reviewed
Apr 9, 2026
| .to_used_value(Au::from_f64_px(space_width)) | ||
| }) | ||
| }; | ||
| let resolve_word_spacing_for_font = |_font: &FontRef| word_spacing; |
Contributor
There was a problem hiding this comment.
You can drop this lambda
Contributor
Author
There was a problem hiding this comment.
You mean the underscore? I added it cause compiler was complaining;
servo-layout v0.1.0 (/home/sabb/servo/components/layout)
warning: unused variable: `font`
--> components/layout/flow/inline/text_run.rs:476:46
|
476 | ...rd_spacing_for_font = |font: &FontRef| word_spacing;
| ^^^^ help: if this is intentional, prefix it with an underscore: `_font`
|
= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default
Contributor
There was a problem hiding this comment.
No, I mean remove resolve_word_spacing_for_font.
Contributor
Author
There was a problem hiding this comment.
Do i drop them here too?
components/layout/flow/inline/text_run.rs:517:37
|
517 | ...ome(resolve_word_spacing_for_font(&font));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope
error[E0425]: cannot find function `resolve_word_spacing_for_font` in this scope
--> components/layout/flow/inline/text_run.rs:549:41
|
549 | ...ome(resolve_word_spacing_for_font(&font));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope
Contributor
Author
There was a problem hiding this comment.
@Loirooriol Do you still want me to drop it?
Cause FontAndScriptInfo requires the word_spacing field, perhaps you have something else to replace this let word_spacing = Some(resolve_word_spacing_for_font(&font)); with?
Contributor
There was a problem hiding this comment.
Yes, drop it everywhere, and drop the 2nd let word_spacing. Just use the original word_spacing variable, but wrap it in Some().
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Loirooriol
reviewed
Apr 10, 2026
| let parent_style = self.inline_styles.style.borrow().clone(); | ||
| let inherited_text_style = parent_style.get_inherited_text().clone(); | ||
| let font_size = parent_style.get_font().font_size.computed_size().into(); | ||
| let word_spacing = inherited_text_style.word_spacing.to_used_value(font_size); |
Contributor
There was a problem hiding this comment.
You can add Some() here, and drop the variables below.
Contributor
Author
There was a problem hiding this comment.
Done.
Any other changes needed?
Signed-off-by: Sabb <sarafaabbas@gmail.com>
Loirooriol
approved these changes
Apr 10, 2026
servo-highfive
removed
the
S-awaiting-review
mrobinson
approved these changes
Apr 10, 2026
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
word-spacingto resolve percentages against the computedfont-size, instead of the size of a space.Testing:
tests/wpt/tests/css/css-text/word-spacing/word-spacing-percent-001.htmlis now passingFixes: #43997