[web] Fix compositing order when adding paragraph tags #17645
Conversation
| @@ -524,6 +537,7 @@ class BitmapCanvas extends EngineCanvas { | |||
| rootElement.append(paragraphElement); | |||
| } | |||
| _children.add(paragraphElement); | |||
| _allocateNewCanvas(); | |||
There was a problem hiding this comment.
We used to do this before, but then discovered that this killed performance on some of the early demos, such as the weather app demo, so we changed it to always put text on top. Can you please add a benchmark prior to submitting this PR that interleaves text with drawing commands so we understand the performance hit of this change? I imagine once @mdebbar's text on canvas work is done we'll be able to use fillText for all text.
There was a problem hiding this comment.
A while ago introduced childOverDraw which makes sure that once we switch to using <p> tags we don't go back requesting extra canvas(s) to draw text. The weather app had drawImage+drawText pattern which is already handled since images are drawn using <img> tags all the time now. The only benchmark we could artificially create that would show regression would be of the pattern drawText, drawComplexPath, drawText that i have not seen in apps yet. One option : A list of material cards that have draw path + images + text is a more realistic benchmark but would not be effected by this change.
There was a problem hiding this comment.
Ah, right, we're creating them lazily. Ok, I guess it's not critical then, just nice-to-have.
| // Any drawing operations after these tags should allocate a new canvas, | ||
| // instead of drawing into earlier canvas. | ||
| void _allocateNewCanvas() { | ||
| _canvasPool.allocateExtraCanvas(); |
There was a problem hiding this comment.
Let's rename _allocateNewCanvas and allocateExtraCanvas to something like "closeCurrentCanvas", since they do not allocate a new canvas. Otherwise it can be confusing when looking at the profiles that allocateExtraCanvas takes 1 nanosecond, but drawCircle takes 10 milliseconds.
There was a problem hiding this comment.
Done. Thanks for suggestion.
|
Merging on infra failure. |
* Add test and dartfmt * Update golden locks * Addressed review comments * Fix lint error (unused widthConstraint) * Add maxDiffRatePercent for text diff on mac/linux
Related to issue : Text shows over animated widget in the presence of a network image when in Stack flutter#53078