Align: try to keep baseline offset

[maurerdietmar]

Keep the baseline offset when height does not change (horizontal align).
This avoids alignment problems when used inside a horizontal Flex box.

Signed-off-by: Dietmar Maurer [email protected]

[jneem]

Thanks for the PR, and sorry for the slow response!

I do have one minor concern, which is that this will also trigger for vertical aligns that just happen to tightly fit their contents. Is this the desired behavior? I suspect not, because it would cause the baseline to suddenly change if the layout changes by just one pixel.

[maurerdietmar]

I do have one minor concern, which is that this will also trigger for vertical aligns that just happen to tightly fit their contents. Is this the desired behavior?

You are right, this is not correct. Unfortunately I have no idea how to fix it correctly.

[jneem]

It looks like height_factor will always be either None or Some(1.0), and it will be Some(1.0) for horizontal aligns.

(I don't understand why the current code is like this, and I'd be happy to accept a simplification.)

[maan2003]

I don't understand why the current code is like this

maybe because of inspiration from flutter?

https://github.com/flutter/flutter/blob/77d935af4db863f6abd0b9c31c7e6df2a13de57b/packages/flutter/lib/src/widgets/basic.dart#L1968-L1976

[jneem]

Good point, although flutter allows changing the factor and we don't.

That brings up a question: what would be the desired behavior of the baseline if height_factor were Some(2.)?

[maurerdietmar]

That brings up a question: what would be the desired behavior of the baseline if height_factor were Some(2.)?

No idea. I guess a similar problem arises if we add padding to a widget?

Anyway, here is a small program to show the problem:

align.rs.txt

[maurerdietmar]

Yes, the same problem occurs with padding. My example just uses .padding(0.) to show the effect.

padding-baseline.rs.txt

[jneem]

Probably the desired behavior is that the baseline is propagated through the padding, right? (I haven't done much text layout in druid so I'm not really sure.) In that case, I think the right behavior for align is to preserve the baseline whenever height_factor.is_some(). Does that make sense?

[maurerdietmar]

Probably the desired behavior is that the baseline is propagated through the padding, right? (I haven't done much text layout in druid so I'm not really sure.) In that case, I think the right behavior for align is to preserve the baseline whenever height_factor.is_some(). Does that make sense?

Yes. I just run a few tests with flutter. and flutter handles this pretty well. They have a special method computeDistanceToActualBaseline() in there box render model:

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/box.dart
(see comment about Intrinsics and Baselines)

Please note that this should works with all kind of boxes (Align, Padding, Container, ...)

[maurerdietmar]

I am also not sure the current flex.rs layout is correct. For example, any_use_baseline seems wrong to me. I would use:

@ -674,7 +675,9 @@ impl<T: Data> Widget<T> for Flex<T> {
         for child in &mut self.children {
             match child {
                 Child::Fixed { widget, alignment } => {
-                    any_use_baseline &= *alignment == Some(CrossAxisAlignment::Baseline);
+                    if let Some(alignment) = *alignment {
+                        any_use_baseline &= alignment == CrossAxisAlignment::Baseline;
+                    }

[maurerdietmar]

The solution for padding widget seems to be trivial:

diff --git a/druid/src/widget/padding.rs b/druid/src/widget/padding.rs
index 63d84e14..f5680551 100644
--- a/druid/src/widget/padding.rs
+++ b/druid/src/widget/padding.rs
@@ -106,6 +106,10 @@ impl<T: Data, W: Widget<T>> Widget<T> for Padding<T, W> {
         let my_size = Size::new(size.width + hpad, size.height + vpad);
         let my_insets = self.child.compute_parent_paint_insets(my_size);
         ctx.set_paint_insets(my_insets);
+        let baseline_offset = self.child.baseline_offset();
+        if baseline_offset > 0f64 {
+            ctx.set_baseline_offset(baseline_offset + insets.y1);
+        }
         trace!("Computed layout: size={}, insets={:?}", my_size, my_insets);
         my_size
     }

[maurerdietmar]

Probably the desired behavior is that the baseline is propagated through the padding, right? (I haven't done much text layout in druid so I'm not really sure.) In that case, I think the right behavior for align is to preserve the baseline whenever height_factor.is_some(). Does that make sense?

Just tested with different height_factor. It works if height_factor >= 1.0.

I am unsure what a height_factor < 1.0 is supposed to do?

diff --git a/druid/src/widget/align.rs b/druid/src/widget/align.rs
index ae13c3a7..8fd31f5e 100644
--- a/druid/src/widget/align.rs
+++ b/druid/src/widget/align.rs
@@ -129,6 +129,14 @@ impl<T: Data> Widget<T> for Align<T> {
 
         let my_insets = self.child.compute_parent_paint_insets(my_size);
         ctx.set_paint_insets(my_insets);
+        if self.height_factor.is_some() {
+            let baseline_offset = self.child.baseline_offset();
+            if baseline_offset > 0f64 {
+                ctx.set_baseline_offset(baseline_offset + extra_height/2.0);
+            }
+        }
+
+
         trace!(
             "Computed layout: origin={}, size={}, insets={:?}",
             origin,

[maurerdietmar]

I can also make it work for Container (see code below).

But how is it supposed to work with SizedBox and AspectRatioBox? Any ideas?

diff --git a/druid/src/widget/container.rs b/druid/src/widget/container.rs
index 6cc10817..79a76335 100644
--- a/druid/src/widget/container.rs
+++ b/druid/src/widget/container.rs
@@ -198,6 +198,11 @@ impl<T: Data> Widget<T> for Container<T> {
 
         let my_insets = self.child.compute_parent_paint_insets(my_size);
         ctx.set_paint_insets(my_insets);
+        let baseline_offset = self.child.baseline_offset();
+        if baseline_offset > 0f64 {
+            ctx.set_baseline_offset(baseline_offset + border_width);
+        }
+
         trace!("Computed layout: size={}, insets={:?}", my_size, my_insets);
         my_size
     }

[jneem]

I think this makes sense (although I'm not super familiar with this code). But I'm confused by:

• why is any_use_baseline not updated in the "measure flex children" loop below?
• is there any way to even set alignment for a fixed child? I only see it in FlexParams.

[maurerdietmar]

• why is any_use_baseline not updated in the "measure flex children" loop below?

just updated the code for that. It now defaults to "false", and it is only set when a child uses baseline alignment.

• is there any way to even set alignment for a fixed child? I only see it in FlexParams.

I guess not. But we can add this later if someone needs it?

[jneem]

Sure, makes sense. I was just confused that we were only checking alignment for those children that couldn't have it set.

[jneem]

This looks good to me now, thanks again for the PR!

But, you will need to mollify CI before I can merge it...