fix baseline offset for sized widget

[maurerdietmar]

Baseline offset is relative to the bottom of a widget. Current code
assumes that the widget has exactly the required height. For sized
widgets, where there is more vertical space, the baseline is totally
wrong. This patch series considers the additional space to adjust the
baseline offset.

Signed-off-by: Dietmar Maurer dietmar@proxmox.com

[maurerdietmar]

There are more widgets that needs to be fixed, i.e. TextBox. Attached an example to show the problem:

align.rs.txt

[maurerdietmar]

Oh, I already added the TextBox fix. Just want to notice that I do
not understand the original code:

 let baseline_off = layout_baseline
-            - (self.inner.child_size().height - self.inner.viewport_rect().height())
-            + textbox_insets.y1;

Why does the baseline depend on the viewport_rect?

[jneem]

Good question, I'm not sure (@cmyr?).

I mean, if the Scroll is scrolled, then the text baseline will appear to move. But I'm not sure we want to actually use that scrolled baseline for aligning anything -- it would cause weird layout changes as you scroll.

[cmyr]

this was a long time ago, but I believe this is just trying to figure out where in the overall widget the text is.

let baseline_off = layout_baseline
- (self.inner.child_size().height - self.inner.viewport_rect().height())

The layout_baseline that we have is relative to the top of the inner TextComponent. Druid tracks baselines relative to the bottom of the layout rect; to make this conversion we need to know where exactly the top of the TextComponent is, relative to our own layout rect. So:

• we have a point relative to the top of a TextComponent that is in a Padding in a Scroll (Scroll<Padding<TextComponent>>)
• self.inner.child_size().height is the total height of the Padding widget, inside the scroll. This can be larger than the scroll.
• self.inner.viewport_rect().height() is the height of the visible region; it does not account for the scroll position
• so then (self.inner.child_size().height - self.inner.viewport_rect().height()) is the total height of the content of the textbox, - the height of the visible region.

so what I think this is trying to do is to account for any possible scroll that is currently not visible, in order to adjust the baseline up to the bottom of the visible region. I am not sure it is right. Staring at it right now, I would expect it to be self.inner.viewport_rect().max_y(), instead of height()? But also I was quite careful with a lot of this code, and especially things like this I would have often tested manually.

This is easy enough, though: if you have a textbox that can scroll vertically, and it is baseline aligned, do weird things happen to the layout when you scroll that textbox?

[maurerdietmar]

so what I think this is trying to do is to account for any possible scroll that is currently not visible, in order to adjust the baseline up to the bottom of the visible region. I am not sure it is right. Staring at it right now, I would expect it to be self.inner.viewport_rect().max_y(), instead of height()? But also I was quite careful with a lot of this code, and especially things like this I would have often tested manually.

I simply don't get why you want to account for any possible scroll that is currently not visible. Make no sense to me.
The baseline should not depend on the scroll position at all.

This is easy enough, though: if you have a textbox that can scroll vertically, and it is baseline aligned, do weird things happen to the layout when you scroll that textbox?

Yes. I have postend an example above where you can see the effects.

[jneem]

Ah, I was reading it wrong, sorry. The baseline calculation doesn't depend on the scroll offset, just the height of the viewport. The effect of the current code is to put the baseline of the text box at the baseline of the first line of text, when the viewport is scrolled all the way to the top. I don't know if that's the desired behavior, but it's what it does.

2022-01-13.16-25-27.mp4

[maurerdietmar]

I don't know if that's the desired behavior, but it's what it does.

IMHO that behavior is really strange (Please compare with my patch applied)