Fixing GetLineBoundaries with hard line break at the end#188868
Conversation
So <Home> and <End> in text editing work correctly
There was a problem hiding this comment.
Code Review
This pull request refactors the web paragraph layout logic to track the index of a hard line break (hardLineBreakStart) instead of a boolean flag, and corrects baseline calculations for strut styles. A review comment identifies a critical bug in getLineBoundary where soft-wrapped lines return incorrect boundaries at the line transition index, offering a clear counter-example and test case to address the issue.
| if (line.allLineTextRange.start <= codepointPosition && | ||
| line.allLineTextRange.end > codepointPosition) { | ||
| return ui.TextRange(start: line.allLineTextRange.start, end: line.allLineTextRange.end); | ||
| line.hardLineBreakStart >= codepointPosition) { | ||
| return ui.TextRange(start: line.allLineTextRange.start, end: line.hardLineBreakStart); |
There was a problem hiding this comment.
Bug: Incorrect Line Boundary for Offsets at Line Start
When codepointPosition is at the boundary between two lines (i.e., equal to the start of the second line, which is also the end of the first line), and the first line does not end with a hard line break, line.hardLineBreakStart for the first line is equal to line.allLineTextRange.end.
Because of the >= operator in line.hardLineBreakStart >= codepointPosition, the first line will match codepointPosition, and getLineBoundary will incorrectly return the boundary of the first line instead of the second line.
Counter-Example:
For a paragraph with text "Hello World" wrapped into two lines:
- Line 1:
start = 0,end = 6,hardLineBreakStart = 6("Hello ") - Line 2:
start = 6,end = 11,hardLineBreakStart = 11("World")
If we query getLineBoundary(6) (the index of 'W'):
- For Line 1:
0 <= 6and6 >= 6are both true, so it matches Line 1 and returns[0, 6). - However, index 6 is
'W', which belongs to Line 2. It should return[6, 11).
Proposed Test Case:
test('getLineBoundary handles soft-wrapped multiline text correctly at line boundaries', () {
final paragraphStyle = WebParagraphStyle(
fontFamily: 'Arial',
fontSize: 20,
);
const text = 'Hello World';
final builder = WebParagraphBuilder(paragraphStyle);
builder.addText(text);
final WebParagraph paragraph = builder.build();
paragraph.layout(const ui.ParagraphConstraints(width: 60)); // Force wrap between Hello and World
final ui.TextRange boundary = paragraph.getLineBoundary(6);
expect(boundary, const ui.TextRange(start: 6, end: 11));
}); if (line.allLineTextRange.start <= codepointPosition &&
(line.allLineTextRange.end > codepointPosition ||
(line.lineNumber == lines.length - 1 && line.allLineTextRange.end == codepointPosition))) {
return ui.TextRange(start: line.allLineTextRange.start, end: line.hardLineBreakStart);References
- Search for counter-examples: Identify scenarios or edge cases that the proposed code does not handle. If a counter-example is found, propose a test case to demonstrate the gap. (link)
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
No description provided.