Fix HTML renderer does not render border #34812
Conversation
|
Gold has detected about 1 new digest(s) on patchset 1. |
|
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. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. |
| final double top = math.min(line.top, line.bottom); | ||
| final double width = line.width.abs(); | ||
| final double height = line.height.abs(); | ||
| if (line.width == 0 || line.height == 0) { |
There was a problem hiding this comment.
Is this check necessary? This condition seems to be guaranteed by toStraightLine, see
There was a problem hiding this comment.
Good catch! I updated the PR to remove this redundant check.
| drawRRect(rrect, paint); | ||
| return; | ||
| } | ||
| // Use drawRect for straight line paths painted with a zero strokeWidth |
There was a problem hiding this comment.
I'm not sure I actually understand why we want to use drawRect instead of just drawing the path anyway and just changing the stroke width to 1.0. Can you clarify?
There was a problem hiding this comment.
@eyebrowsoffire
This is related to this part of my PR description:
"The implementation choice is to draw a filled rectangle with a 1 width or a 1 height. This choice is done for optimization purposes (compared to relying on drawing the line on an Web canvas)."
With the HTML renderer, drawRectwill be rendered as <div>, drawing the path will be rendered as a <canvas>. As the use cases for this issue seem to be mainly using a separator on long list, my guess is that it is cheaper to have the browser rendering many <div> insteaf of many <canvas>, but I can be wrong here.
There was a problem hiding this comment.
No, that's a good call. We want to avoid creating unnecessary canvas elements if a drawing call can be expressed using an HTML element. However, do note that this does not guarantee a div. It depends on the Paint and other environmental factors whether a div or a canvas will be used.
af46a44 to
d3e0b0c
Compare
|
Gold has detected about 1 new digest(s) on patchset 1. |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
@yjbanov |
|
Yeah, I did it, but it's having trouble registering it. I just triaged it again. 🤞 this is pass now. |
* 071f888 [web] Compile tests with sound null safety (flutter/engine#36169) * 73c9b87 Roll Skia from ce330bb13879 to 962fd1f9abfc (2 revisions) (flutter/engine#36170) * 2fb44a2 Fix HTML renderer does not render border (flutter/engine#34812)
Description
This PRs fixes an issue where the HTML renderer was not able to draw straight lines when stroke width is zero (For more information see the demo here: flutter/flutter#46339 (comment)).
The implementation choice is to draw a filled rectangle with a 1 width or a 1 height. This choice is done for optimization purposes (compared to relying on drawing the line on an Web canvas).
Related Issue
Fixes flutter/flutter#46339
Tests
Adds 1 test.