[WebParagraph] Support for more styles, placeholders, decorations, etc #172853
Conversation
Moving the test file
Test still not working
Drawing paragraph on Canvas2D (not working)
Drawing paragraph on Canvas2D (not working)
Drawing paragraph on Canvas2D (not working)
on DrawImage
No bidi, no hard line breaks
(Required flattening the style ranges)
(it breaks but somewhere down the road)
All tests workin
(with some tests)
Can build, layout and simple paint (not tested)
The basic test works the same way as SkParagraph
(not many of them supported)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a significant refactoring that adds a lot of functionality to WebParagraph, bringing it closer to the SkParagraph implementation. The code is well-structured, separating layout, painting, and data models. The new implementation seems much more robust and feature-complete.
I've found a couple of potential issues regarding an unsafe method and a fixed-size canvas that could lead to bugs. Additionally, many new public APIs are missing documentation, which is required by the Flutter style guide. I've added a few comments highlighting examples of where documentation should be added.
| ClusterRange convertTextToClusterRange(TextRange textRange) { | ||
| final int clusterStart = textToClusterMap[textRange.start]!; | ||
| final int clusterEnd = textToClusterMap[textRange.end - 1]!; | ||
| return ClusterRange(start: clusterStart, end: clusterEnd + 1); | ||
| } |
There was a problem hiding this comment.
This method can throw an exception if textRange is empty. For an empty range, textRange.start == textRange.end. If textRange.end is N, then textRange.end - 1 will be N - 1, which could be a valid index for a different part of the text, or an invalid index if N is 0. This can lead to incorrect behavior or exceptions.
While call sites currently seem to check for empty ranges, it would be safer to handle this case within the method itself to make it more robust.
ClusterRange convertTextToClusterRange(TextRange textRange) {
if (textRange.isEmpty) {
final int clusterIndex = textToClusterMap[textRange.start]!;
return ClusterRange(start: clusterIndex, end: clusterIndex);
}
final int clusterStart = textToClusterMap[textRange.start]!;
final int clusterEnd = textToClusterMap[textRange.end - 1]!;
return ClusterRange(start: clusterStart, end: clusterEnd + 1);
}
| final DomOffscreenCanvas _paintCanvas = createDomOffscreenCanvas(500, 500); | ||
| final paintContext = _paintCanvas.getContext('2d')! as DomCanvasRenderingContext2D; |
There was a problem hiding this comment.
The offscreen canvas _paintCanvas is created with a fixed size of 500x500. If a text block or decoration is larger than this, it will be clipped when rendered on this canvas. This could lead to visual bugs for large text or long lines.
The canvas should be dynamically resized to fit the content being drawn onto it before each use.
| abstract class Painter { | ||
| Painter(); | ||
|
|
||
| void fillTextCluster(ExtendedTextCluster webTextCluster, bool isDefaultLtr); | ||
| void paintTextCluster(ui.Canvas canvas, ui.Rect sourceRect, ui.Rect targetRect); | ||
|
|
||
| void fillShadow(ExtendedTextCluster webTextCluster, ui.Shadow shadow, bool isDefaultLtr); | ||
| void paintShadow(ui.Canvas canvas, ui.Rect sourceRect, ui.Rect targetRect); | ||
|
|
||
| void paintBackground( | ||
| ui.Canvas canvas, | ||
| LineClusterBlock block, | ||
| ui.Rect sourceRect, | ||
| ui.Rect targetRect, | ||
| ); | ||
|
|
||
| void fillDecorations(LineClusterBlock block, ui.Rect sourceRect); | ||
| void paintDecorations(ui.Canvas canvas, ui.Rect sourceRect, ui.Rect targetRect); | ||
| } |
| @@ -76,21 +104,83 @@ class WebParagraphStyle implements ui.ParagraphStyle { | |||
| } | |||
| } | |||
|
|
|||
| enum StyleElements { background, shadows, decorations, text } | |||
| class TextRange extends _RangeStartEnd { | ||
| TextRange({required int start, required int end}) : super(start, end); | ||
|
|
||
| TextRange.collapsed(super.offset) : super.collapsed(); | ||
|
|
||
| TextRange.zero() : super.zero(); | ||
|
|
||
| TextRange clone() { | ||
| return TextRange(start: start, end: end); | ||
| } | ||
|
|
||
| @override | ||
| // No need to override hashCode, since _RangeStartEnd already does it. | ||
| // ignore: hash_and_equals | ||
| bool operator ==(Object other) { | ||
| if (identical(this, other)) { | ||
| return true; | ||
| } | ||
| return other is TextRange && super == other; | ||
| } | ||
|
|
||
| @override | ||
| String toString() { | ||
| return '[$start:$end)'; | ||
| } | ||
|
|
||
| TextRange translate(int offset) { | ||
| return TextRange(start: start + offset, end: end + offset); | ||
| } | ||
|
|
||
| static TextRange intersectTextRange(TextRange a, TextRange b) { | ||
| return TextRange(start: math.max(a.start, b.start), end: math.min(a.end, b.end)); | ||
| } | ||
| } |
This is the second version of WebParagraph. It includes pretty much all SkParagraph functionality except struts and justifications (eventually they will be implemented, too).
Part of #172561