Right aligned backspace bug #25229
Right aligned backspace bug #25229
Conversation
|
@HansMuller Does it seem reasonable that this could just be an off by one error as in my code change? Can you explain what |
zoechi
added
a: text input
justinmc
force-pushed
the
text-field-right-backspace
branch
2 times, most recently
from
48c436d to
27f788e
Compare
|
@HansMuller This is ready for a real review now. Several tests were expecting incorrect behavior, so that's what all the lines changed in the test are. |
justinmc
force-pushed
the
text-field-right-backspace
branch
from
27f788e to
73a3110
Compare
There was a problem hiding this comment.
As far as I can tell, this all looks correct. LGTM
It would be helpful to add some additional explanation to the TextAffinity enum doc, particularly about white space, implicit newlines ("forced line breaks"), genuine newlines, and bidi text. Many of the corner cases covered by the tests probably deserve a sentence or two. Given that, other places that would benefit from the clarification, like the TextPosition class, could remind readers to "See Also" the TextAfinity class for more information about cursor positioning...
Updating the TextAffinity et al docs could be a separate PR.
|
|
||
| // When text wraps on its own, getOffsetForCaret disambiguates between the | ||
| // end of one line and start of next using affinity. | ||
| text = 'aaaaaaaa'; // Just enough to wrap one character down to second line |
There was a problem hiding this comment.
I assume that this wraps because SIZE_OF_A * 8 (112) is wider than the layout width of 100, specified below. Maybe add a comment on the painter.layout line explaining what's up here.
| // Correctly moves through second line with upstream affinity. | ||
| caretOffset = painter.getOffsetForCaret(const ui.TextPosition(offset: 4, affinity: TextAffinity.upstream), ui.Rect.zero); | ||
| expect(caretOffset.dx, closeTo(42.0, 0.0001)); | ||
| // Check that getOffsetForCaret handles being at the end when affinity is |
There was a problem hiding this comment.
I think it would be better to explain what the test is checking, like the previous test does (and the next one as well). Something like: Given a one-line right aligned string, positioning the cursor at offset 0 means that it appears at the "end" of the string, at x = 0.
|
|
||
| // Correctly handles multiple trailing newlines. | ||
| // Correctly handles multiple trailing newlines with the same offset |
There was a problem hiding this comment.
As before, best to explain what we expect here.
|
@HansMuller I just improved the comments and cleaned the tests up a bit. I'll open a PR in the engine to improve the TextAffinity docs and reference it here. |
|
@GaryQian Heads up that I'm changing similar code in text_painter.dart to what you just changed. I just merged your changes into this PR and I think it went well though. I think you and I were partially fixing the same bug in |
| final double caretEnd = box.end; | ||
| final double dx = box.direction == TextDirection.rtl ? caretEnd - caretPrototype.width : caretEnd; | ||
| return Offset(dx, box.top); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| // Get the Offset of the cursor (in pixels) based off the near edge of the |
There was a problem hiding this comment.
May want to distinguish between logical vs physical pixels, as 'pixels' may be ambiguous. In this case I think everything should be logical pixels.
| @@ -417,12 +417,15 @@ class TextPainter { | |||
| // Unicode value for a zero width joiner character. | |||
| static const int _zwjUtf16 = 0x200d; | |||
|
|
|||
| // Get the Offset of the cursor (in pixels) based off the near edge of the | |||
|
@GaryQian I clarified "logical pixels" in the docs like you mentioned, but I couldn't find a better way to check for newlines other than the integer value 10. It looks like |
Closes #25199
In an input with
textAlign: TextAlign.rightwith the cursor in the middle of the text:Deleting until the end of the line caused the cursor to jump to the far right:
This PR fixes it so the cursor stays in the correct place, so in this example it would be:
I also encountered related bugs with multiline inputs while working on this. When going to an empty new line with text align set to right, the cursor would jump back over to the far left. These bugs seem to be fixed with this change as well.