Skip to content
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

[SuperTextField] Fix horizontal aligment (Resolves #668) #716

Merged
merged 13 commits into from
Aug 14, 2022

Conversation

angelosilvestre
Copy link
Collaborator

[SuperTextField] Fix horizontal aligment. Resolves #668

Horizontal alignment was not working, as shown in #509

There are two different situations:

Multiline text fields: The RenderParagraph has a maxWidth constraint. In Flutter TextPainter, when maxWidth differs from minWidth, it layouts the paragraph again using maxIntrinsicWidth as maxWidth, so the RenderParagraph doesn't use all the available width and the alignment doesn't work as expected. As a result, the scrollbar is displayed next to the text instead of being displayed at the right edge of the text field.

The relevant lines are here:

https://github.com/flutter/flutter/blob/ca6cecf034d9c18fcb6ea70309cc855e353aa0f8/packages/flutter/lib/src/painting/text_painter.dart#L630-L652

I wrapped SuperText with a SizedBox with infinity maxWidth, so it forces the RenderParagraph to use all available width.

Singleline text fields: The text field has a horizontal scrollview, which gives an infinity maxWidth to RenderParagraph. In this case, wrapping with the SizedBox won't help.

I wrapped the SingleChildScrollView with an Align widget, with an alignment based on the text alignment.

@matthew-carroll
Copy link
Contributor

@angelosilvestre I didn't quite follow the explanation in the PR. Specifically, I'm wondering why alignment used to work, but it doesn't any more. My guess is that the super_text_layout work broke it. But that still begs the question: what was the change that broke it, and if that change was in super_text_layout, should we change super_text_layout instead of adding more widgets to SuperTextField?

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll The main problem is that, to be able to respect the text alignment, SuperText needs to take all available width. Instead, it takes only its maxIntrinsicWidth (this is done in TextPainter).

Because of this, the text is aligned only within SuperText's bounds, and not within SuperTextField's bounds.

The alignment stopped working in b2e0dc7, although center alignment of a single line text field wasn't working before that.

The text was being displayed inside a _FillWidthIfConstrained. The alignment was working because, with this widget, the text was taking all the available width.

In b2e0dc7, the text was wrapped with a IntrinsicWidth, which caused the text to stop taking all the available width.

I can try to avoid using the Align widget, but I think we still need to use a SizedBox or reimplement _FillWidthIfConstrained.

@matthew-carroll
Copy link
Contributor

Let's bring back _FillWidthIfConstrained along with a doc comment that explains why we need it. I probably deleted it because I couldn't remember why we were using it, and we didn't have any tests that failed.

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll I implemented FillWidthIfConstrained a bit differently because it was causing problems when using computeDryLayout (even implementing computeDryLayout on RenderSuperTextLayout).

I checked it again, and the text alignment was working with _FillWidthIfConstrained only when SuperTextField was multiline.

I think we still need to use Align (or something similar). When SuperTextField is single line, it has a horizontal SingleChildScrollView. Because of this, it has an infinity maxWidth, so we can't force the text to take the available width.

We also can't position the text inside the scrollview.

@matthew-carroll
Copy link
Contributor

@angelosilvestre it looks like this PR has conflicts

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll I resolved the conflits.

markNeedsLayout();
}

double? _viewportWidth = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that this property is given an initial value of 0? It looks like the constructor always assigns an initial value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initialization isn't needed. I removed it.

double? viewportWidth,
}) : _viewportWidth = viewportWidth;

set viewportWidth(double? value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a dart doc that explains what viewport this is, and why we care.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also changed the property name to make it more clear.

maxWidth: constraints.maxWidth,
maxHeight: constraints.maxHeight,
);
} else if (_viewportWidth != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care whether the viewport scrolls vertically vs horizontally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only care about horizontal scrollables.

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matthew-carroll matthew-carroll merged commit 6704f8e into superlistapp:main Aug 14, 2022
@angelosilvestre angelosilvestre deleted the issue-668 branch August 16, 2022 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SuperTextField] - Horizontal alignment is broken
2 participants