[canvaskit] fix text background, foreground, color; add text style tests #23800
Conversation
yjbanov
force-pushed
the
ck-text-colors
branch
from
c504f37 to
c2ef85a
Compare
| _defaultTextForeground.setColorInt( | ||
| skStyle.color?.value ?? 0xFF000000, | ||
| ); |
There was a problem hiding this comment.
I have a bad feeling about this mutation of a static default style. Doesn't it leak the color to other paragraphs?
There was a problem hiding this comment.
We haven't found a better way to do this yet. JavaScript doesn't have value types so any structs we pass to WASM end up extra objects allocated in the heap creating GC pressure. So in a few cases where we know it's safe, we keep a shared statically allocated WASM object and use it to pass data to CanvasKit without allocating extra objects. There are a few more examples in canvaskit_api.dart where we do this, such as:
I tested this specifically by invoking it twice in a row and I didn't see it leaking to another text style. But I agree, that this can indeed be dangerous. You have to know that internally Skia is taking the object by value rather than by pointer. We should avoid this pattern unless there's a strong reason to use it.
There was a problem hiding this comment.
One case I can think of is:
- First paragraph:
foreground == nullskStyle.color.value == GREEN
- Second paragraph:
foreground == nullskStyle.color.value == null
Expected: The first paragraph is green, the second paragraph has the default color (black?).
Actual: The green color leaked to the second paragraph.
That said, I agree with the GC pressure issue. Maybe add a quick comment here explaining the possible leak and the reason it was done this way?
There was a problem hiding this comment.
Ok, I'm convinced that this is risky, so I added a test to make sure we don't leak styles: https://github.com/flutter/goldens/blob/76847ac130d9dc2ec4188694daff8fa16735a7d8/engine/web/canvaskit_text_styles_do_not_leak.png
yjbanov
force-pushed
the
ck-text-colors
branch
4 times, most recently
from
c9325f7 to
375b134
Compare
yjbanov
changed the title
yjbanov
force-pushed
the
ck-text-colors
branch
from
fe80788 to
a3f185d
Compare
| addPlaceholder, | ||
| } | ||
|
|
||
| String _fontFamilyOrFallback(String? fontFamily) { |
There was a problem hiding this comment.
This shouldn't be necessary. Roboto is always added to the global fallback fonts
| void pop() { | ||
| if (_styleStack.length <= 1) { | ||
| // The top-level text style is paragraph-level. We don't pop it off. | ||
| return; |
yjbanov
force-pushed
the
ck-text-colors
branch
from
cef4adc to
cb2460f
Compare
…24142) * add ffi_allocation_patch.dart to libraries.yaml 2 (#23954) * Use `runes` to get code units in CanvasKit. (#24024) * [windows] Honor only valid resize requests (#23990) * Load FlutterLoader when creating FlutterEngineGroup (#23980) * [canvaskit] remove the DOM node of unrendered platform view (#24001) * update dart to 2.12.0-259.8.beta * [web] Fix svg based stroke rendering. (#23969) * [canvaskit] fix text background, foreground, color; add text style tests (#23800) * [canvaskit] fix text background, foreground, familyFallback; add style tests * add leak test * update goldens_lock.yaml * remove solo * Warn when popping out of empty text style stack * [web] Fix alignment issue in rich paragraphs (#23965) * [web] Implement CanvasParagraph.getLineBoundary (#24037) * [web] Fix text alignment when transform + tight constraints + DOM rendering (#24036) * update licenses_third_party golden Co-authored-by: Daco Harkes <[email protected]> Co-authored-by: Harry Terkelsen <[email protected]> Co-authored-by: Kaushik Iska <[email protected]> Co-authored-by: xster <[email protected]> Co-authored-by: Yegor <[email protected]> Co-authored-by: Ferhat <[email protected]> Co-authored-by: Mouad Debbar <[email protected]>
…sts (flutter#23800) * [canvaskit] fix text background, foreground, familyFallback; add style tests * add leak test * update goldens_lock.yaml * remove solo * Warn when popping out of empty text style stack
3 participants
Fix text background/foreground and font family fallback in CanvasKit renderer. Add text style tests.
Fixes flutter/flutter#73473
Fixes flutter/flutter#74336