-
Notifications
You must be signed in to change notification settings - Fork 56
Enable floats and other advanced layouts #421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dd3a1d6 to
62d4418
Compare
e7a038b to
257754b
Compare
f6e5cef to
e55c5cb
Compare
90fdd2e to
486eb53
Compare
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
Signed-off-by: Nico Burns <[email protected]>
taj-p
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay on the review - this PR is actually way more complex than I expected. Kudos on being able to wrangle the complexity of line breaking to support this custom behaviour.
| .break_remaining(max_advance.unwrap_or(f32::MAX)); | ||
| } | ||
|
|
||
| /// Apply alignment to the layout relative to the specified container width or full layout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update this documentation?
| if self.state.layout_max_advance >= f32::MAX { | ||
| self.layout.data.alignment_width = full_width; | ||
| for line in &mut self.lines.lines { | ||
| line.metrics.inline_max_coord = line.metrics.inline_min_coord + width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my information, why do we want to overwrite the inline_max_coord for a line with the maximum width + inline_min_coord? What utility does it provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I don't think this is correct anymore actually. The basic logic of "if a width constraint is infinite, then set it to a computed value" (+ we treat f32::MAX as infinite) makes sense. But I think it should be done on a line-by-line basis rather than using the layout_max_advance now.
The key thing this is trying to avoid is Parley performing alignment against an infinite width. That not only positions text completely incorrectly, but also causes performance issues.
| &self.data.styles | ||
| } | ||
|
|
||
| /// Returns available width of the layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please expand the documentation of available_width, width, and full_width so that it's clear how each differ?
|
|
||
| // If laying out with infinite width constraint, then set all lines' "max_width" | ||
| // to the measured width of the longest line. | ||
| if self.state.layout_max_advance >= f32::MAX { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if self.state.layout_max_advance >= f32::MAX { | |
| if self.state.layout_max_advance == f32::MAX { |
Is the >= useful here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is if someone passes f32::INFINITY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please add a test that appreciably uses the new functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are some draft code tests that I'm still a little unsatisfied with, but hopefully they help you in crafting the tests you think best exercise the new API:
#[test]
fn custom_break_lines_circle_layout() {
let mut env = TestEnv::new(test_name!(), None);
*env.max_screenshot_size() = Some(20 * 1024);
let text = "Curving text is easier when Parley lets us steer every line. ".repeat(8);
let mut builder = env.ranged_builder(text.as_str());
builder.push_default(StyleProperty::FontSize(22.0));
let mut layout = builder.build(text.as_str());
let line_height = measure_line_height(&layout);
let diameter = 360.0;
apply_circle_breaking(&mut layout, diameter, line_height);
env.rendering_config()
.size
.replace(Size::new(diameter as f64, diameter as f64));
env.check_layout_snapshot(&layout);
}
#[test]
fn custom_break_lines_with_excluded_region() {
let mut env = TestEnv::new(test_name!(), None);
let text = "\
Parley can now flow around shapes.
When a floating shape occupies the left margin,
subsequent lines shift right until the note ends.
After the float, text snaps back to the full width.
";
let mut builder = env.ranged_builder(text);
builder.push_default(StyleProperty::FontSize(18.0));
builder.push_inline_box(InlineBox {
id: 1,
kind: InlineBoxKind::CustomOutOfFlow,
index: 0,
width: 120.0,
height: 90.0,
});
let mut layout = builder.build(text);
let inline_boxes = layout.inline_boxes().to_vec();
let total_width = 420.0;
apply_exclusion_layout(&mut layout, &inline_boxes, total_width, 14.0);
env.rendering_config()
.size
.replace(Size::new(total_width as f64, layout.height() as f64));
env.check_layout_snapshot(&layout);
}
const MIN_LINE_WIDTH: f32 = 24.0;
fn measure_line_height(layout: &Layout<ColorBrush>) -> f32 {
let mut measurement = layout.clone();
measurement.break_lines().break_next().unwrap();
let height = measurement
.lines()
.next()
.map(|line| line.metrics().line_height)
.unwrap();
height
}
fn apply_circle_breaking(
layout: &mut Layout<ColorBrush>,
diameter: f32,
mut line_height_hint: f32,
) {
let radius = diameter * 0.5;
let mut breaker = layout.break_lines();
let mut line_top = 0.0f64;
let mut needs_setup = true;
loop {
if needs_setup {
let (line_x, line_width) =
circle_band_for_y(radius, diameter, line_top, line_height_hint);
let state = breaker.state_mut();
state.set_layout_max_advance(diameter);
state.set_line_x(line_x);
state.set_line_max_advance(line_width.max(MIN_LINE_WIDTH));
state.set_line_y(line_top);
needs_setup = false;
}
match breaker.break_next() {
Some(YieldData::LineBreak(line_break)) => {
line_height_hint = line_break.line_height;
line_top += line_break.line_height as f64;
needs_setup = true;
}
Some(YieldData::InlineBoxBreak(_)) => {}
Some(YieldData::MaxHeightExceeded(data)) => {
panic!("Unexpected max-height break at {}", data.line_height);
}
None => break,
}
}
}
fn circle_band_for_y(
radius: f32,
diameter: f32,
line_top: f64,
line_height: f32,
) -> (f32, f32) {
if line_height <= 0.0 {
return (0.0, diameter);
}
let top = line_top as f32;
if top >= diameter {
return (0.0, diameter);
}
let band_center = top + line_height * 0.5;
if !(0.0..=diameter).contains(&band_center) {
return (0.0, diameter);
}
let dy = (band_center - radius).abs();
if dy >= radius {
return (0.0, diameter);
}
let half_width = (radius * radius - dy * dy).max(0.0).sqrt();
let left = radius - half_width;
let width = (half_width * 2.0).max(MIN_LINE_WIDTH);
(left, width.min(diameter - left))
}
#[derive(Clone, Copy)]
struct FloatRegion {
bottom: f32,
width: f32,
}
fn apply_exclusion_layout(
layout: &mut Layout<ColorBrush>,
inline_boxes: &[InlineBox],
total_width: f32,
gap: f32,
) {
let mut breaker = layout.break_lines();
let mut line_top = 0.0f64;
let mut active_float: Option<FloatRegion> = None;
let mut needs_setup = true;
loop {
if needs_setup {
if let Some(float) = active_float {
if line_top as f32 >= float.bottom {
active_float = None;
}
}
let (line_x, max_width) = if let Some(float) = active_float {
let offset = (float.width + gap).min(total_width - MIN_LINE_WIDTH);
(offset, (total_width - offset).max(MIN_LINE_WIDTH))
} else {
(0.0, total_width)
};
let state = breaker.state_mut();
state.set_layout_max_advance(total_width);
state.set_line_x(line_x);
state.set_line_max_advance(max_width);
state.set_line_y(line_top);
needs_setup = false;
}
match breaker.break_next() {
Some(YieldData::LineBreak(line_break)) => {
line_top += line_break.line_height as f64;
needs_setup = true;
}
Some(YieldData::InlineBoxBreak(box_break)) => {
let inline_box = &inline_boxes[box_break.inline_box_index];
active_float = Some(FloatRegion {
bottom: line_top as f32 + inline_box.height,
width: inline_box.width,
});
let state = breaker.state_mut();
let offset = (inline_box.width + gap).min(total_width - MIN_LINE_WIDTH);
state.set_line_x(offset);
state.set_line_max_advance((total_width - offset).max(MIN_LINE_WIDTH));
}
Some(YieldData::MaxHeightExceeded(data)) => {
panic!("Unexpected max-height break at {}", data.line_height);
}
None => break,
}
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can definitely add some tests. I agree that an exclusion region makes sense for this.
| /// for laying out the box. | ||
| /// | ||
| /// They can be used to implement advanced layout modes such as CSS's `float` | ||
| CustomOutOfFlow, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the CustomOutOfFlow test could take the form of an exclusion region (like one of the draft tests I mentioned above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll have to remind me why we want to make these changes to the snapshots? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because alignment is now performed using the width constraint set for the line rather than the computed width of the line. This means that if you have a line of text that doesn't take up the whole width of the box it is being laid out into and it is e.g. right-aligned then it will (IMO correctly) align to the right-edge of the box.
Previously the alignment width was set manually, but the behaviour implemented in many of the tests was to align to max(computed line length) (maxing over the length of all lines in the layout). But if none of the lines get to the very end (because of unbreakable words) then it would align to the edge of the longest line rather than the edge of the box.
|
|
||
| let box_will_be_appended = next_x <= max_advance || self.state.line.x == 0.0; | ||
| if height_contribution > self.state.line_max_height && box_will_be_appended { | ||
| return self.max_height_break_data(height_contribution); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this block can ever be entered with the current API (since line_max_height isn't ever set)
| } | ||
|
|
||
| // TODO: this method does not handle mixed direction text at all. | ||
| pub(crate) fn calculate_content_widths(&self) -> ContentWidths { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect different content widths for layouts with and without out of flow boxes?
This test currently fails because we always add the width of the box:
parley/parley/src/layout/data.rs
Line 579 in 00fdba7
| running_max_width += ibox.width; |
#[test]
fn out_of_flow_inline_boxes_do_not_affect_content_widths() {
let mut env = TestEnv::new(test_name!(), None);
let text = "Hello world";
let without_box = {
let mut layout = env.ranged_builder(text).build(text);
layout.break_all_lines(None);
layout.calculate_content_widths()
};
let mut builder = env.ranged_builder(text);
builder.push_inline_box(InlineBox {
id: 99,
kind: InlineBoxKind::OutOfFlow,
index: 0,
width: 400.0,
height: 20.0,
});
let mut layout = builder.build(text);
layout.break_all_lines(None);
let with_box = layout.calculate_content_widths();
assert_eq!(
without_box.min, with_box.min,
"out-of-flow inline boxes must not change min-content width"
);
assert_eq!(
without_box.max, with_box.max,
"out-of-flow inline boxes must not change max-content width"
);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, no. Well spotted. I think this issue might have introduced when trying to resolve merge conflicts.
As an aside: in CSS, position:absolute (represented here by OutOfFlow boxes) can affect the scroll height/width of their parent, but not the layout (so nothing that is in scope for Parley).
| while let Some(data) = self.break_next() { | ||
| match data { | ||
| YieldData::LineBreak(line_break_data) => { | ||
| self.state.line_y += line_break_data.line_height as f64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please check my understanding? By moving this logic into break_remaining, then break_next suffers a breaking change in behaviour because default iteration no longer advances line_y. In other words, this test will fail:
#[test]
fn break_lines_preserves_default_line_positions() {
let mut env = TestEnv::new(test_name!(), None);
let text = "One line\nTwo line\nThree line\nFour line";
let mut layout = env.ranged_builder(text).build(text);
{
let mut breaker = layout.break_lines();
while let Some(result) = breaker.break_next() {
match result {
YieldData::LineBreak(_) => {}
YieldData::InlineBoxBreak(_) => panic!("unexpected inline box break"),
YieldData::MaxHeightExceeded(_) => panic!("unexpected max-height break"),
}
}
}
let mut baselines = layout.lines().map(|line| line.metrics().baseline);
let mut previous = baselines
.next()
.expect("expected layout to produce at least one line");
for baseline in baselines {
assert!(
baseline > previous,
"expected baseline {baseline} to be greater than previous baseline {previous}"
);
previous = baseline;
}
}Are we happy with that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you been able to confirm that these changes do not regress performance (either via parley_bench or in Blitz)?
Related PRs
Cluster::from_point_exactmethod for hit-testing spans #447container_widtha required alignment parameter #283Introduction
This PR is aimed at enabling Blitz to implement CSS floats on top of Parley. However, the changes made are not float-specific, and will also enable other advanced layouts such as those that have "exluded regions" or want to lay out text into a complex shape.
Parley's existing architecture
Layout::break_all_lines(max_advance)to perform line breaking, Parley does already have a lower-levelLayout::break_lines()function which returns aBreakLinesstruct, on which you can callBreakLines::next(). Each call toBreakLines::next()lays out a single line and then returns. This PR extends the functionality of this method so that in addition returning when a line-break occurs, it will also return when:max_line_heightis exceededLinestruct hasmin_coordandmax_coordfields.Changes Made
Per-line bounding box
max_advanceWhen using the
BreakLinesstruct directly, themax_advance(max width) can now be configured on a per-line basis. This makes it possible to create layouts with lines broken to different lengths. Additionally, it is valid to adjust themax_advancemidway through line-breaking a line (it is advised to ensure that the existing content of the line still fits, although nothing terrible will happen if you don't (the contents will just overflow)).Automatically determined (per-line) alignment width
Related to the per-line max-advances, the width used for alignment is now per-line and determined automatically and is equal to the
max_advancefor that line (at the time that the line is "committed"). This means that each line is aligned within it's own width bound, separately from the other liens. This is really the only way to sensibly align lines of differing widths, but making alignment width automatic had previously been discussed as being desirable anyway.Added
max_line_heightEach line now has a
max_line_height. If at any point the line exceeds this height, control flow will be yielded fromBreakLines::next(), allowing the user to find a new, larger space to lay out into. By default,max_line_heightis set tof32::MAXand thus has no effect.Customisable per-line x- and y-offset
Parley previously assumed that every line started at x=0 and that the start of the next line would be immediately below the previous line. Parley now allows an x and y offset to be manually configured for each line. This does not affect line breaking, but is used when reporting the positions of glyphs and for selection.
The x offset is in addition to and stored separately from the offset generated by alignment.
Added
InlineBoxKindenumA new
kind: InlineBoxKindfield has been added theInlineBoxstruct, where InlineBoxKind is defined as:InlineBoxKind::InFlowrepresents the existing kind of inline box that is is laid out in-flow with text like adisplay: inline-blockbox in CSS.InlineBoxKind::OutOfFlowis assigned a position during layout in exactly the same way as anInFlowbox. However it does not take up space or affect the position of other items (glyphs or boxes) in the layout. This corresponds to aposition: absolutebox in CSS. Blitz was previously representing this as a zero-sized box, but I have taken the opportunity here to represent it explicitly.InlineBoxKind::CustomOutOfFlowis the box kind for a floated box. Parley does not attempt to lay these out at all. When it encountered a box of this kind it yields control flow back to the caller who then responsible for positioning the box, adjusting the line's position/size, and then resuming layout (details below).Tasks