Fix text selection line height #11353
Conversation
There was a problem hiding this comment.
Please fix the grammar of this comment while you're here...
There was a problem hiding this comment.
Please document whatever this is.
In particular, the documentation should include the following details:
- What the value represents.
- What purpose is served by exposing this value here.
- What the units of the value are.
- When the value might change.
- How to get notified when the value changes.
|
This needs a test for the bugs that are fixed here. |
There was a problem hiding this comment.
Why does it not require the layout to be updated? e.g. if the font size was animated up, wouldn't the toolbar need to animate its position also?
There was a problem hiding this comment.
Rephrase. Was referring to the callee not needing to be laid out
…ositions and sizes for toolbar and selection handles
xster
force-pushed
the
fix-text-selection-line-height
branch
from
2ef66dc to
18afda1
Compare
|
Added a test for text_selection widget to check plumbing |
| /// The height of a typical text line in logical pixels. | ||
| /// | ||
| /// This height is based on properties of the text's style such as | ||
| /// font size etc and could change when those properties change. |
There was a problem hiding this comment.
But how would a user of this API know when that happens?
For example, if you change the font-size of a text field that is showing its toolbar, will the toolbar's position be adjusted?
There was a problem hiding this comment.
Generically, the user wouldn't. It's mostly in line with the other getters in this file or that are downstream from the text painter like text or textAlign.
In the context of its relation with the selection overlay, it seems like all property changes of this renderable are coordinated via the EditableTextState which creates the widgets that populates this renderable in the first place by _updateOrDisposeSelectionOverlayIfNeeded etc when anything changes.
There was a problem hiding this comment.
text and textAlign are upstream of the painter. You set them. As far as I can tell, this is the first getter that would expose layout-based data.
Generally, this is an anti-pattern. If another object reads the layout, e.g. if the parent render object uses the preferredLineHeight in some way, then this render object changes its configuration, the parent won't update, until randomly the other child is dirtied, then suddenly it will update. We go to extreme lengths to make it possible to read the size, the intrinsic dimensions, etc, in such a way that the right objects get dirtied when the values change to avoid these problems.
I don't see anything in this file which causes the EditableTextState to be updated when the layout changes.
|
We discussed this patch in person and if I'm not mistaken we agreed to go a rather different route, so I'm going to close this PR for now to take it off the review queue. We can open a new PR when we have a new patch to review. |
3 participants
Fix a bug introduced when calculating the text line height for the text selection overlay #11224 (comment).
Also fix #11261.
Expose the preferredLineHeight from RenderEditable because the information is needed for the text UI.