Resolve mismatch between label width and alignment (#727)

If the alignment was set previously, and the `CrossAxisAlignment` was
not `Start`, the width Parley believed we were in wouldn't match the
width used for Masonry layout. This would lead to weird behaviour.

See the diff for the test images in
0a68159
for more context

masonry/src/text/text_layout.rs

@@ -255,6 +255,9 @@ impl TextLayout {
     }
 
     /// Set the [`Alignment`] for this layout.
+    ///
+    /// This alignment can only be meaningful when a
+    /// [maximum width](Self::set_max_advance) is provided.
     pub fn set_text_alignment(&mut self, alignment: Alignment) {
         if self.alignment != alignment {
             self.alignment = alignment;
@@ -313,20 +316,43 @@ impl TextLayout {
         &self.layout
     }
 
-    /// The size of the laid-out text, excluding any trailing whitespace.
+    /// The size of the region the text should be drawn in,
+    /// excluding any trailing whitespace if present.
+    ///
+    /// Should be used for the drawing of non-interactive text (as the
+    /// trailing whitespace is selectable for interactive text).
     ///
     /// This is not meaningful until [`Self::rebuild`] has been called.
     pub fn size(&self) -> Size {
         self.assert_rebuilt("size");
-        Size::new(self.layout.width().into(), self.layout.height().into())
+        Size::new(
+            self.layout_width(self.layout.width()).into(),
+            self.layout.height().into(),
+        )
     }
 
     /// The size of the laid-out text, including any trailing whitespace.
     ///
+    /// Should be used for the drawing of interactive text.
+    ///
     /// This is not meaningful until [`Self::rebuild`] has been called.
     pub fn full_size(&self) -> Size {
         self.assert_rebuilt("full_size");
-        Size::new(self.layout.full_width().into(), self.layout.height().into())
+        Size::new(
+            self.layout_width(self.layout.full_width()).into(),
+            self.layout.height().into(),
+        )
+    }
+
+    /// If performing layout `max_advance` to calculate text alignment, the only
+    /// reasonable behaviour is to take up the entire available width.
+    ///
+    /// The coherent way to have multiple items laid out on the same line is for them to
+    /// be inside the same text layout object "region". This is currently deferred.
+    /// As an interim solution, we allow multiple items to be on the same line if the `max_advance` wasn't used
+    /// (and therefore the text alignment argument is effectively ignored).
+    fn layout_width(&self, width: f32) -> f32 {
+        self.max_advance.unwrap_or(width)
     }
 
     /// Return the text's [`LayoutMetrics`].

masonry/src/widget/label.rs

@@ -20,8 +20,9 @@ use crate::{
     RegisterCtx, TextEvent, Update, UpdateCtx, Widget, WidgetId,
 };
 
-// added padding between the edges of the widget and the text.
-pub(super) const LABEL_X_PADDING: f64 = 2.0;
+/// Added padding between each horizontal edge of the widget
+/// and the text in logical pixels.
+const LABEL_X_PADDING: f64 = 2.0;
 
 /// Options for handling lines that are too wide for the label.
 #[derive(Debug, Clone, Copy, PartialEq)]
@@ -202,7 +203,9 @@ impl Widget for Label {
                 .rebuild(font_ctx, layout_ctx, &self.text, self.text_changed);
             self.text_changed = false;
         }
-        // We ignore trailing whitespace for a label
+        // We would like to ignore trailing whitespace for a label.
+        // However, Parley doesn't make that an option when using `max_advance`.
+        // If we aren't wrapping words, we can safely ignore this, however.
         let text_size = self.text_layout.size();
         let label_size = Size {
             height: text_size.height,
@@ -265,7 +268,7 @@ mod tests {
     use crate::assert_render_snapshot;
     use crate::testing::TestHarness;
     use crate::theme::{PRIMARY_DARK, PRIMARY_LIGHT};
-    use crate::widget::{Flex, SizedBox};
+    use crate::widget::{CrossAxisAlignment, Flex, SizedBox};
 
     #[test]
     fn simple_label() {
@@ -291,6 +294,35 @@ mod tests {
         assert_render_snapshot!(harness, "styled_label");
     }
 
+    #[test]
+    /// A wrapping label's alignment should be respected, regardkess of
+    /// its parent's alignment.
+    fn label_alignment_flex() {
+        fn base_label() -> Label {
+            Label::new("Hello")
+                .with_text_size(10.0)
+                .with_line_break_mode(LineBreaking::WordWrap)
+        }
+        let label1 = base_label().with_text_alignment(Alignment::Start);
+        let label2 = base_label().with_text_alignment(Alignment::Middle);
+        let label3 = base_label().with_text_alignment(Alignment::End);
+        let label4 = base_label().with_text_alignment(Alignment::Start);
+        let label5 = base_label().with_text_alignment(Alignment::Middle);
+        let label6 = base_label().with_text_alignment(Alignment::End);
+        let flex = Flex::column()
+            .with_flex_child(label1, CrossAxisAlignment::Start)
+            .with_flex_child(label2, CrossAxisAlignment::Start)
+            .with_flex_child(label3, CrossAxisAlignment::Start)
+            .with_flex_child(label4, CrossAxisAlignment::Center)
+            .with_flex_child(label5, CrossAxisAlignment::Center)
+            .with_flex_child(label6, CrossAxisAlignment::Center)
+            .gap(0.0);
+
+        let mut harness = TestHarness::create_with_size(flex, Size::new(80.0, 80.0));
+
+        assert_render_snapshot!(harness, "label_alignment_flex");
+    }
+
     #[test]
     fn line_break_modes() {
         let widget = Flex::column()

masonry/src/widget/prose.rs

@@ -11,13 +11,16 @@ use vello::peniko::BlendMode;
 use vello::Scene;
 
 use crate::text::{ArcStr, TextBrush, TextWithSelection};
-use crate::widget::label::LABEL_X_PADDING;
 use crate::widget::{LineBreaking, WidgetMut};
 use crate::{
     AccessCtx, AccessEvent, BoxConstraints, CursorIcon, EventCtx, LayoutCtx, PaintCtx,
     PointerEvent, QueryCtx, RegisterCtx, TextEvent, Update, UpdateCtx, Widget, WidgetId,
 };
 
+/// Added padding between each horizontal edge of the widget
+/// and the text in logical pixels.
+const PROSE_X_PADDING: f64 = 2.0;
+
 /// The prose widget is a widget which displays text which can be
 /// selected with keyboard and mouse, and which can be copied from,
 /// but cannot be modified by the user.
@@ -136,7 +139,7 @@ impl Prose {
 impl Widget for Prose {
     fn on_pointer_event(&mut self, ctx: &mut EventCtx, event: &PointerEvent) {
         let window_origin = ctx.widget_state.window_origin();
-        let inner_origin = Point::new(window_origin.x + LABEL_X_PADDING, window_origin.y);
+        let inner_origin = Point::new(window_origin.x + PROSE_X_PADDING, window_origin.y);
         match event {
             PointerEvent::PointerDown(button, state) => {
                 if !ctx.is_disabled() {
@@ -223,7 +226,7 @@ impl Widget for Prose {
             None
         } else if bc.max().width.is_finite() {
             // TODO: Does Prose have different needs here?
-            Some(bc.max().width as f32 - 2. * LABEL_X_PADDING as f32)
+            Some(bc.max().width as f32 - 2. * PROSE_X_PADDING as f32)
         } else if bc.min().width.is_sign_negative() {
             Some(0.0)
         } else {
@@ -234,11 +237,11 @@ impl Widget for Prose {
             let (font_ctx, layout_ctx) = ctx.text_contexts();
             self.text_layout.rebuild(font_ctx, layout_ctx);
         }
-        // We ignore trailing whitespace for a label
-        let text_size = self.text_layout.size();
+        // We include trailing whitespace for prose, as it can be selected.
+        let text_size = self.text_layout.full_size();
         let label_size = Size {
             height: text_size.height,
-            width: text_size.width + 2. * LABEL_X_PADDING,
+            width: text_size.width + 2. * PROSE_X_PADDING,
         };
         bc.constrain(label_size)
     }
@@ -255,7 +258,7 @@ impl Widget for Prose {
             scene.push_layer(BlendMode::default(), 1., Affine::IDENTITY, &clip_rect);
         }
         self.text_layout
-            .draw(scene, Point::new(LABEL_X_PADDING, 0.0));
+            .draw(scene, Point::new(PROSE_X_PADDING, 0.0));
 
         if self.line_break_mode == LineBreaking::Clip {
             scene.pop_layer();
@@ -289,4 +292,45 @@ impl Widget for Prose {
     }
 }
 
-// TODO - Add tests
+// TODO - Add more tests
+#[cfg(test)]
+mod tests {
+    use parley::layout::Alignment;
+    use vello::kurbo::Size;
+
+    use crate::{
+        assert_render_snapshot,
+        testing::TestHarness,
+        widget::{CrossAxisAlignment, Flex, LineBreaking, Prose},
+    };
+
+    #[test]
+    /// A wrapping prose's alignment should be respected, regardkess of
+    /// its parent's alignment.
+    fn prose_alignment_flex() {
+        fn base_label() -> Prose {
+            // Trailing whitespace is displayed when laying out prose.
+            Prose::new("Hello  ")
+                .with_text_size(10.0)
+                .with_line_break_mode(LineBreaking::WordWrap)
+        }
+        let label1 = base_label().with_text_alignment(Alignment::Start);
+        let label2 = base_label().with_text_alignment(Alignment::Middle);
+        let label3 = base_label().with_text_alignment(Alignment::End);
+        let label4 = base_label().with_text_alignment(Alignment::Start);
+        let label5 = base_label().with_text_alignment(Alignment::Middle);
+        let label6 = base_label().with_text_alignment(Alignment::End);
+        let flex = Flex::column()
+            .with_flex_child(label1, CrossAxisAlignment::Start)
+            .with_flex_child(label2, CrossAxisAlignment::Start)
+            .with_flex_child(label3, CrossAxisAlignment::Start)
+            .with_flex_child(label4, CrossAxisAlignment::Center)
+            .with_flex_child(label5, CrossAxisAlignment::Center)
+            .with_flex_child(label6, CrossAxisAlignment::Center)
+            .gap(0.0);
+
+        let mut harness = TestHarness::create_with_size(flex, Size::new(80.0, 80.0));
+
+        assert_render_snapshot!(harness, "prose_alignment_flex");
+    }
+}

xilem/examples/http_cats.rs

@@ -13,8 +13,8 @@ use winit::window::Window;
 use xilem::core::fork;
 use xilem::core::one_of::OneOf3;
 use xilem::view::{
-    button, flex, image, portal, prose, sized_box, spinner, worker, Axis, CrossAxisAlignment,
-    FlexExt, FlexSpacer,
+    button, flex, image, inline_prose, portal, prose, sized_box, spinner, worker, Axis, FlexExt,
+    FlexSpacer,
 };
 use xilem::{Color, EventLoop, EventLoopBuilder, TextAlignment, WidgetView, Xilem};
 
@@ -102,12 +102,10 @@ impl HttpCats {
                     portal(sized_box(info_area).expand_width()).flex(1.),
                 ))
                 .direction(Axis::Horizontal)
-                .cross_axis_alignment(CrossAxisAlignment::Fill)
                 .must_fill_major_axis(true)
                 .flex(1.),
             ))
-            .must_fill_major_axis(true)
-            .cross_axis_alignment(CrossAxisAlignment::Fill),
+            .must_fill_major_axis(true),
             worker(
                 worker_value,
                 |proxy, mut rx| async move {
@@ -164,8 +162,8 @@ impl Status {
         let code = self.code;
         flex((
             // TODO: Reduce allocations here?
-            prose(self.code.to_string()),
-            prose(self.message),
+            inline_prose(self.code.to_string()),
+            inline_prose(self.message),
             FlexSpacer::Flex(1.),
             // TODO: Spinner if image pending?
             // TODO: Tick if image loaded?
@@ -199,7 +197,6 @@ impl Status {
             prose("Copyright ©️ https://http.cat      ").alignment(TextAlignment::End),
         ))
         .main_axis_alignment(xilem::view::MainAxisAlignment::Start)
-        .cross_axis_alignment(CrossAxisAlignment::Fill)
     }
 }
 

xilem/examples/mason.rs

@@ -61,9 +61,7 @@ fn app_logic(data: &mut AppData) -> impl WidgetView<AppData> {
     fork(
         flex((
             flex((
-                label("Label")
-                    .brush(Color::REBECCA_PURPLE)
-                    .alignment(TextAlignment::Start),
+                label("Label").brush(Color::REBECCA_PURPLE),
                 label("Bold Label").weight(TextWeight::BOLD),
                 // TODO masonry doesn't allow setting disabled manually anymore?
                 // label("Disabled label").disabled(),