Improve spacing and justification model, stop mutating advances in-place - #738
Conversation
1f0e42a to
50feabe
Compare
There was a problem hiding this comment.
The "amet" on the second line moves about a pixel to the left here, which I believe is now correct. This is due to the hanging trailing space I mention in the PR description. There are a few more of these diffs.
There was a problem hiding this comment.
It isn't immediately clear to me why this doesn't also apply to elit, on the next line.
There was a problem hiding this comment.
Perhaps I was a bit loose with my wording. I specifically mean overflowing trailing whitespace (not just hanging). The trailing space of the second line overflows the layout width, but as it is hung it doesn't need to wrap. We weren't counting justification opportunities correctly in that case.
There was a problem hiding this comment.
This now correctly gets 2px of spacing to the right of the "2" on the first line, due to the letter-spacing. Compare: https://developer.mozilla.org/en-US/play?id=xUzPsQvQPN%2BH8KQQW7EbahNXi3IY8eaVqbAFvHLg3VTBnrMs808%2B%2B3QRSyJZqxVXKxSQLgAquwN2IvsJ.
| @@ -1519,13 +1538,15 @@ fn commit_line<B: Brush>( | |||
| .is_some_and(|atom| atom.characters()[0].info.whitespace().is_space_or_nbsp()) | |||
| { | |||
| num_spaces = num_spaces.saturating_sub(1); | |||
There was a problem hiding this comment.
I think this should actually continue walking until the first non-trailing space atom (though it doesn't regress here).
There was a problem hiding this comment.
I think that Canva's Parley fork does actually make that change, so we'd welcome it being made here.
| if is_word_separator(whitespace) { | ||
| gaps.after += self.spacing.word; | ||
|
|
||
| if atom.shaped_clusters_range().end != self.justification.line_end_cluster { | ||
| gaps.after += self.justification.amount_per_opportunity; | ||
| } | ||
| } |
There was a problem hiding this comment.
The is_word_separator for applying word spacing might need a bit of tuning. This is getting into real edge-case territory. In the following text, there's a grapheme directly following "aa" made of [U+0D4E|space]. Browsers apply word spacing after that grapheme, even though it start with a space.
aaൎ bb cc dd
With this PR, we render it as follows.
There was a problem hiding this comment.
And for posterity, this PR rendering that with letter-spacing instead.
There was a problem hiding this comment.
even though it start with a space
Doesn't it not start with a space, or am I missing something?
There was a problem hiding this comment.
Yeah, you're right, I should've written "even though it doesn't start with a space." This branch checks the logically first character, and that'll require more tuning to fully match browsers.
| fn glyphs_with_spacing<'a, B: Brush>( | ||
| run: Run<'a, B>, | ||
| clusters: Range<u32>, | ||
| spacing: LineSpacing, | ||
| ) -> impl Iterator<Item = Glyph> + Clone + use<'a, B> { |
There was a problem hiding this comment.
The iterator code in this function is... not very nice. Very open to suggestions.
|
WPT results with Blitz against this look decent: WPT ResultsWPT: blitz PR#627 (parley@9c41a4d) vs parley#738 (50feabe)Baseline: 10563 PASS / 14527 FAIL / 141 CRASH / 11908 SKIP Net +33 passes. Results verified deterministic (identical across two runs). Fixed (FAIL -> PASS): 53
Regressed (PASS -> FAIL): 20
|
|
Am I right in thinking that this means that one would no longer be able to randomly access glyph positions (you'd have to iterate through the line)? |
Very cool! Thanks for running the tests. Will be fun to dive into exactly what those newly-failing tests are doing.
Yeah, that's right. It's similar to how the atoms and graphemes are cursor-based. If a consumer knows they'll be accessing the glyphs a lot, or want random access, I suppose they could materialize them into an allocation (or maybe we should think about letting I think separating storage of shaped results from shaping-irrelevant styling is nice architecturally, as it will help to allow performant rejustification or animating spacing and such. |
tomcur
changed the title
I was more thinking about selection performance. Presumably that will regress here? Although I guess only for justified text if we're smart. And possibly not sufficiently that it matters.
Agreed! But this is orthogonal to not storing the data at all, right? |
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I definitely like the direction. Some of the decisions inside are a little bit non-obvious to me, as expanded in the comments. But overall, this is a reasonable start, and I trust you to get it to the finish line/bring whatever needs further discussion to OH tomorrow.
| pub(crate) word_spacing: f32, | ||
| /// Additional letter spacing. | ||
| pub(crate) letter_spacing: f32, | ||
| /// Additional spacing inserted between this run's atoms. |
There was a problem hiding this comment.
It's a little bit surprising to me that this would be atoms, but I haven't reasoned through it.
I guess the point is that HarfRust will be told that we need spacing, so will try to minimise the size of atoms anyway?
There was a problem hiding this comment.
Grapheme boundaries are potential places to put spacing, but if graphemes are ligated such that a shaped cluster spans across the boundary, we cannot put any spacing there. Atom boundaries are exactly where grapheme and shaped cluster boundaries agree.
There was a problem hiding this comment.
Yeah, it makes sense. I just wonder if this is something whether that ligation decision in itself would have been valid (that is, is this like reshaping after breaking?)
There was a problem hiding this comment.
In case of nonzero letter-spacing, CSS indeed specifies optional ligatures should not form (and in #731 we follow that).
| // Justified alignment doesn't apply to the last line of a paragraph | ||
| // (`BreakReason::None`), (`BreakReason::Explicit`) or if there are no whitespace | ||
| // gaps to adjust. In that case, start-align, i.e., left-align for LTR text and | ||
| // right-align for RTL text. | ||
| if matches!(line.break_reason, BreakReason::None | BreakReason::Explicit) | ||
| || line.num_spaces == 0 | ||
| { | ||
| if is_rtl { | ||
| line.metrics.offset += free_space; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Not needed now, but this is where https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/text-align-last would apply, I think.
| if atom.characters()[0].info.whitespace().is_space_or_nbsp() { | ||
| self.line.num_spaces += 1; | ||
| } |
There was a problem hiding this comment.
I know I keep asking this same question again, but could you remind me why this is correct (over a version which says if any of the component grapheme clusters are a space).
Or really, I just you to claim that you yourself are convinced of that!
There was a problem hiding this comment.
I think there isn't necessarily a correct or incorrect. Gecko and Blink do different things, but after diving into it again I think perhaps we should change our logic a bit further. See also the prepend+space case in #738 (comment).
Of Gecko and Blink, the former seems most self-consistent, and its stretch opportunities are quite close to the predicate "the atom's last character is a space." Perhaps we should adopt that (in a follow-up PR).
Blink appears to be somewhat buggy: it looks like it counts justification opportunities in a different way from how it determines whether a space can be stretched. See https://developer.mozilla.org/en-US/play?id=4ZJ8MG9vK2jKwMlmZ4qH%2BqRp5lQ0QDLeZmJTAOP8sevrRi6i0fW4LgqhLm8BX1vfxM2gKnll7C17pBL4, where, using Chromium, the second space gets stretched based on whether the third space has a prepend or not. With Chromium, that renders as follows on my machine.
Other than that, Blink is quite close to the predicate "the atom's first character is a space," but it relies on HarfBuzz's grapheme clustering, which isn't quite the same as UAX #29 extended grapheme clusters.
| if is_word_separator(whitespace) { | ||
| gaps.after += self.spacing.word; | ||
|
|
||
| if atom.shaped_clusters_range().end != self.justification.line_end_cluster { | ||
| gaps.after += self.justification.amount_per_opportunity; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incidentally, are you aware of fonts with inked space (e.g. Marelle Lignes - https://marelle.forge.apps.education.fr/).
This is some very edge-case stuff, and browsers don't really put much effort into them in this kind of case.
There was a problem hiding this comment.
No, I didn't know of such fonts. I also just learned of the Ethiopic word space (U+1361) which CSS treats as a word separator that can get additional spacing and has a glyph (though Gecko and Blink don't add spacing to it).
It seems that font itself uses some overdraw to be at least somewhat correct in case of spacing.
| /// The total advance of `slice`, i.e., the sum of shaped advances and gaps. | ||
| #[inline] | ||
| pub(crate) fn slice_advance(self, slice: ShapedSlice<'_>) -> f32 { | ||
| if self.is_zero() { |
There was a problem hiding this comment.
It's not immediately obvious to me that all this if spacing.is_zero() { fast path } else {slightly slower path } is especially valuable.
There was a problem hiding this comment.
It isn't immediately clear to me why this doesn't also apply to elit, on the next line.
| if is_word_separator(whitespace) { | ||
| gaps.after += self.spacing.word; |
There was a problem hiding this comment.
What happens with doubled spaces here? Is that correct?
(From a Canva perspective, either is fine so long as justification is right).
There was a problem hiding this comment.
Yeah, I think it is. I'll add a test separately.
| // whether that interacts correctly with justification. In previous versions, that hanging space | ||
| // was miscounted, resulting in justification opportunities being undercounted. | ||
| // | ||
| // The large word spacing makes it more obvious whether gaps between the words are equal. |
There was a problem hiding this comment.
I would have thought that a small word spacing would lead to that, as a higher proportion of the gap would be due to justification? But maybe I'm missing what this is actually testing.
There was a problem hiding this comment.
This is testing that an overflowing space is redistributed correctly over all interior spaces (it wasn't, previously). Having the word spacing helps ensure the space actually overflows, and the amount that's redistributed is specifically the portion of that overflowing space that's inside the line box (the rest was already hung).
I've updated the wording in the test somewhat (and also tweaked the numbers a bit).
There was a problem hiding this comment.
I think the proof which makes the most sense is to have the word which goes on the second line just be really long, but maybe that's a different question?
But I'll be honest, I still haven't grokked what you're saying. It's not critical though.
…nd returning glyphs
2b6b35c to
af52bea
Compare
<!-- Please ensure that you have reviewed our LLM ("AI") policy at
https://linebender.org/wiki/llm-policy/.
If you did not use any LLM tools, please replace `Unspecified` with
`None`. -->
LLM Contributions: Mostly generated.
Follow-up to
#738 (comment).
This is also what Gecko and Blink do (with whitespace collapsing off).
Compare:
https://developer.mozilla.org/en-US/play?id=gilITaZ%2B0hCJVuNWyCoME9xqzkpiKGs0u2alPNKuvKXYzUEm2XX1efjm7j5qJPFZdiEZU%2FYl7eSQxZlN.
<!--
If our users need to know about this change, please describe that in the
quote block below.
What you write here will be edited by us later - it doesn't need to be
perfect.
If this change doesn't need a changelog entry, please replace the next
line with `**Changelog: None**`.
-->
**Changelog: None**
<!-- Please ensure that you have reviewed our LLM ("AI") policy at
https://linebender.org/wiki/llm-policy/.
If you did not use any LLM tools, please replace `Unspecified` with
`None`. -->
LLM Contributions: Investigation, test.
When justifying a line, this now correctly handles runs of consecutive
trailing spaces. This is follow-up to
linebender#738 (comment).
There's some more that we could/should do here (separately!). E.g., I
think we simply do not need to check the break reason, and that would
also get us to handle `BreakReason::Emergency` correctly for free. We
also don't currently handle consecutive trailing spaces correctly for
either justification or hanging when there's a formatting change inside
that run; something like the following.
```html
<div style="background: #efefef; width: 95px; white-space-collapse: preserve; text-align: justify;">AA BB CC <span style="font-size: 2em;"> </span>DD</div>
```
<!--
If our users need to know about this change, please describe that in the
quote block below.
What you write here will be edited by us later - it doesn't need to be
perfect.
If this change doesn't need a changelog entry, please replace the next
line with `**Changelog: None**`.
-->
**Changelog**
> ### Fixed
>
> - Lines aligned with justification now correctly handle runs of
consecutive trailing spaces.
3 participants
LLM Contributions: Investigation, review, tests.
This replaces the mutating letter/word spacing and justification with a lazy calculation. Stopping mutating is important for reshaping.
The new
spacing.rsmodule encodes the spacing logic. It's similar to the model that Blink and Gecko have, where spacing can be applied visually to the left or right of a shaped cluster. Currently, letter and word spacing are only ever applied visually to the right, following Blink; Gecko, on the other hand, applies these to the logically trailing side. With other CSS styles (liketext-justify: inter-character), spacing would be applied on both sides, so I figured I'd just add it now. It's wired up and Should Just Work ™️.(There's a CSS Working Group discussion considering, e.g., whether letter spacing should be applied on both sides: w3c/csswg-drafts#10193 (comment).)
We now decide whether to apply spacing based on atoms/graphemes, which gets us closer to Blink and Gecko.
For example, a combining acute accent attaches to a preceding space and forms a grapheme. We now space those correctly, even if the constituent characters shape into separate shaped clusters.
letter-spacingwith multi-shaped-cluster graphemeword-spacingwith multi-shaped-cluster graphemeTo see this in your own browser: https://developer.mozilla.org/en-US/play?id=xhx5oFVuorhlPTrMH2xQcApXn32uuxO8hGXLfMMCrjXonfGpgZ13na9OawGhcV4CLixSnStRwgkRGREZ.
I've also fixed one space counting bug, where a hanging trailing space was not counted, but justification would later subtract it, meaning the last interior space would not get any justification. The following has
word-spacingto make the issue a bit more obvious. Note the word "amet" on the first line is too far right in the "before" image. In the "now" image, the two interior spaces are of equal width.This is mostly neutral to a small performance improvement. (Measured with the benches in #737.)
Repeated justification is of course quite a bit faster.
Changelog