Conversation
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
Some initial feedback
|
|
||
| /// A box to be laid out inline with text | ||
| #[derive(PartialEq, Debug, Clone)] | ||
| pub struct InlineBox { |
There was a problem hiding this comment.
Strictly speaking, this is orthogonal to this PR (but it may be useful to implement it here anyway, and it could also land before this PR), but:
InlineBox ought to gain a first_baseline: Option field. If that is Some then the inline box ought to be aligned using that as it's baseline. If it is None then it would fallback to aligning using the full height of the box (aligning to the bottom of the box), which is the current behaviour.
In Blitz, we'd source this value from Taffy and set it here before running text layout.
| let effective_ascent = (run.metrics.ascent - offset).max(0.0); | ||
| let effective_descent = (run.metrics.descent + offset).max(0.0); |
There was a problem hiding this comment.
Not 100% sure, but I think offset should be in the same direction for both of these calculations.
| // Contribute to line metrics considering the offset. | ||
| // Inline boxes sit with bottom at baseline by default, | ||
| // so effective_ascent = height - offset, effective_descent = offset |
There was a problem hiding this comment.
We need to add a first_baseline: Option<f32> field to inline boxes so in the case that the inline box internally contains text, we can align with the first line of text's baseline rather than the bottom of the box.
| VerticalAlign::TextTop => -(line.metrics.ascent - run.metrics.ascent), | ||
| VerticalAlign::TextBottom => line.metrics.descent - run.metrics.descent, |
There was a problem hiding this comment.
We haven't computed line.metrics.ascent here yet? We only finish computing that at the end of this loop.
| line.metrics.ascent = line.metrics.ascent.max(effective_ascent); | ||
| line.metrics.descent = line.metrics.descent.max(effective_descent); |
There was a problem hiding this comment.
I don't think this is sufficient in the general case. As text spans form a tree structure, you can end up with nested offsets (imagine a superscript of a superscript). See #291
Perhaps this is landable like this though? I guess we'd need to test how it performs in practice.
| VerticalAlign::Middle => { | ||
| let x_height = run | ||
| .metrics | ||
| .x_height | ||
| .unwrap_or(run.metrics.ascent * 0.5); |
There was a problem hiding this comment.
CSS spec suggests a fallback of font_size * 0.5 https://www.w3.org/TR/css-inline-3/#baseline-synthesis-fonts
| VerticalAlign::Sub => run.metrics.descent, | ||
| VerticalAlign::Super => -(run.metrics.ascent * 0.4), |
There was a problem hiding this comment.
These really ought to be font-specific values taken from the font's BASE table (in Layout::push_run). And added to the run metrics like x_height currently is.
Inline boxes contribute to max_inline_box_above/max_inline_box_below — so line box height correctly spans cross-item extents (overflow:hidden above + text descent below) Text run half-leading uses run.metrics.line_height instead of the line_height parameter — the parameter is running_line_height which is max(all_cluster_heights, all_inline_box_heights). An inline box with h=135.2 was inflating the text "X"'s half-leading from 3.2 to 51.6, producing the ~174 line_height. This was a pre-existing bug that my inline box tracking change exposed.
|
I think this needs to be split into a few separate PRs:
1, 2, and potentially 3 could potentially all be in the first PR. |
2 participants
No description provided.