Improve white space hanging and justification - #762
Conversation
5f2fcf7 to
1218f0d
Compare
There was a problem hiding this comment.
Prior to this PR, this rendered as follows. Note the NBSP was erroneously hung.
There was a problem hiding this comment.
Prior to this PR, this rendered as follows. Note the NBSP was erroneously hung.
|
Compare the rendering of the non-hanging NBSP with Blink. Both Parley and Blink don't justify it.
For the RTL case, Blink seems somewhat inconsistent, as it then does justify the line-ending NBSP. I think that's just accidental, and may not necessarily be something we want to match. (Note in these screenshots there's some additional
|
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
This definitely looks like an improvement to code clarity. Particular appreciate the variable renames.
A couple of things I think this needs:
- Update
calculate_content_widthsto match new hanging logic. This still only subtracts the single last atom (prev_atom), whereas line breaking now hangs a full run of trailing spaces. With white-space: pre-wrap the intrinsic sizes will disagree with the laid-out width. These ones bite Blitz which relies on content widths matching layout widths, so I'd consider this quite important (probably we should add tests for this). - Ideally the "other whitespace separators" (but could be follow-up)
- [LLM] (relatively minor?) Bidi: trailing space in a non-base-level run now gets stretched.
commit_linepreviously unconditionally excluded the trailing space from num_spaces. Now, whenbidi_level != base_level,hangingis set false and the trailing space falls into theelse if is_word_separator branch, so it's counted as an opportunity and still occupies space.- Affects: e.g. an LTR paragraph whose line ends in an RTL run + space.
- Suggestion: at least excluding the logically last atom from opportunities in that case (as before).
I'd consider 1 blocking, others can be deferred if preferred.
This means we now perform only a single walk to count justification opportunities, where previously we walked forward once, and then had to walk backwards to subtract trailing spaces.
In practice I think this is more work as we're doing 1 walk of the whole line (previously 0) in order to elide a walk of just the trailing whitespace. Although some LLM benchmarks seemed to show that the effect is small.
| atom.filter(|(whitespace, _)| { | ||
| matches!( | ||
| whitespace, | ||
| Whitespace::Space | Whitespace::Tab | Whitespace::Newline | ||
| ) |
There was a problem hiding this comment.
Could we (additionally) include all unicode category Z here? LLM suggest adding a new SpaceSeparator variant to Whitespace to capture this.
There was a problem hiding this comment.
Either way, could we make this a method on Whitespace?
There was a problem hiding this comment.
We could add Zs with a bit of a refactor of Whitespace.
However, from my testing, Gecko and Blink don't seem to hang Zs at the moment.
Firefox and Chromium on my machine don't hang the Em space in the following (try replacing with a bunch of &VeryThinkSpace; and you should see the same thing again).
There was a problem hiding this comment.
Either way, could we make this a method on Whitespace?
Yeah, good idea. I've made it a private function in parley for now (instead of a public method in parley_engine), as by the above apparently the spec isn't strictly followed (yet?).
| // extents) are *not* computed here: they were already accumulated into `self.state.line` as | ||
| // the line was built and are read from there below. | ||
| let mut needs_reorder = false; | ||
| let mut hanging = true; |
There was a problem hiding this comment.
The hanging variable could maybe do with some explanation as a comment. My understanding is that it's iteration state where we iterate back through the line from the logical end and need to track if we're still tracking hanging whitespace at the end or not.
In general, it would be helpful for these variables to distinguish iteration state from outputs (perhaps put them in separate blocks of code?)
There was a problem hiding this comment.
I've moved the variable to be just before the loop and added a comment. It could be a separate code block, but it's already quite deeply nested, so then extracting to a separate function might be called for.
| let slice = self | ||
| .layout | ||
| .data | ||
| .shaped_text | ||
| .run_slice(line_item.index as u32) | ||
| .narrow(line_item.shaped_cluster_range.clone()); |
There was a problem hiding this comment.
I'm a bit confused about how we're skipping trailing whitespace here, but then still checking is cluster_hangs below. How does anything hang if we're skipping hanging clusters?
There was a problem hiding this comment.
I think the code comment just above this code range was simply slightly confusing, as the code range your comment is placed on doesn't skip trailing whitespace. The comment was intended for the full loop.
I've moved the slice binding upward (which also deduplicates some code).
Adds long-single-line benchmarks to measure cases where performance is `O(line length)` These have been used to test linebender#762 **LLM Contributions**: Generated with Fable 5.1 Low **Changelog**: None
1218f0d to
42a816f
Compare
NBSP handling in min/max content widths is covered by the upstream hanging-whitespace rework (linebender#762), so leave calculate_content_widths as on main.
NBSP handling in min/max content widths is covered by the upstream hanging-whitespace rework (linebender#762), so leave calculate_content_widths as on main.
12d9be5 to
8c876c9
Compare
8c876c9 to
7259b48
Compare
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
This looks correct to me. I don't have a strong opinion on whether the Z category should be here or a follow-up.
| // Note: only whitespace hangs at the moment. With something like hanging punctuation, content | ||
| // could also hang past the start edge. |
There was a problem hiding this comment.
With something like hanging punctuation, content could also hang past the start edge.
That would be a different field, right? Just based on this being documented as the amount that hangs from the end edge.
This comment is a little bit confusing.
There was a problem hiding this comment.
Yep, I've improved the comment's clarity somewhat (hopefully).
Measures `Layout::calculate_content_widths`. This function will need some work to match the new line breaking (and linebender#762 in particular), so let's start measuring its performance.
04d832a to
03d9e08
Compare
|
Nico and I spoke out-of-band and thought to do the |
3 participants
LLM Contributions: Investigation, added tests.
On top of #760.
White space across runs (e.g., because of a font size change) can now all hang, like Gecko and Blink. Following CSS Text 4 § 4.3.2 (and Gecko and Blink), non-breaking spaces no longer hang.
This moves the white space hanging logic and justification opportunity counting into
finish_line, i.e., these are now performed when the line is known. This means we now perform only a single walk to count justification opportunities, where previously we walked forward once, and then had to walk backwards to subtract trailing spaces.With more of the logic in one place, it should also be simpler to keep these in sync (previously, the justification count effectively re-implemented the logic of which white space was considered to be hanging).
WPT test results
Note the
line-break: anywheretests were accidentally passing before, as we don't implement that behavior.Performance
This benches mostly neutral on my machine.
Changelog