Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

reland fix canvas drawLine bugs #38949

Merged
merged 2 commits into from
Jan 18, 2023
Merged

reland fix canvas drawLine bugs #38949

merged 2 commits into from
Jan 18, 2023

Conversation

alanwutang11
Copy link
Contributor

@alanwutang11 alanwutang11 commented Jan 17, 2023 •
edited
Loading

There were several bugs with canvas.drawLine when using DOM rendering (this codepath):

  • did not correctly render with negative ui.Offsets passed as parameters
  • did not respect paint.strokeCap
  • did not respect paint.strokeWidth for horizontal and vertical lines

according to the api docs for canvas.drawLine, rendering of lines should be the same regardless of the paint.style.

Previously, however, failing to set paint.style to type stroke (either by omission or setting the paint.style to fill) would cause us to use DOM rendering as specified by this bool, which introduced the above mentioned bugs.

Solution for negative ui.Offsets

instead of using an svg width, height, and viewBox as the bounding box for our path, just remove all of those properties and instead set the svg overflow property to visible.

Changes from the reverted PR to reland

In pathToSvgElement(), the overflow property was only set to visible in 1/2 of the instances pathToSvgElement() was called in the codebase.

Removing the width, height, and viewBox properties from the svg element created by pathToSvgElement() without also setting overflow property to visible for the same svg element caused golden tests to fail.

The fix was to just set the overflow property to visible inside pathToSvgElement() so that every svg created by pathToSvgElement() has an overflow property set to visible.

fixes: flutter/flutter#110070

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 17, 2023
@alanwutang11 alanwutang11 marked this pull request as ready for review January 17, 2023 21:06
yjbanov
yjbanov reviewed Jan 17, 2023
View reviewed changes
lib/web_ui/lib/src/engine/html/dom_canvas.dart
@@ -338,9 +339,7 @@ SVGSVGElement pathToSvgElement(
// In Firefox some SVG typed attributes are returned as null without a
// setter. So we use strings here.
final SVGSVGElement root = createSVGSVGElement()
..setAttribute('width', '${width}px')
..setAttribute('height', '${height}px')
..setAttribute('viewBox', '0 0 $width $height');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this means we can remove the width and height parameters from this function, and transitively all code that computes and passes these values.

yjbanov
yjbanov approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@alanwutang11 alanwutang11 added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 18, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jan 18, 2023

auto label is removed for flutter/engine, pr: 38949, due to - The status or check suite Mac Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@alanwutang11 alanwutang11 merged commit d762142 into flutter:main Jan 18, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023
@engine-flutter-autoroll
d762142bb reland fix canvas drawLine bugs (flutter/engine#38949)
14c4680
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023
@engine-flutter-autoroll
d762142bb reland fix canvas drawLine bugs (flutter/engine#38949)
09970a7
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023
@engine-flutter-autoroll
d762142bb reland fix canvas drawLine bugs (flutter/engine#38949)
040e561
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 18, 2023
@engine-flutter-autoroll
Roll Flutter Engine from f79030440948 to c52b290813bd (29 revisions) (#…
8a58ec5 
…118720)

* 5aee7914c Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter/engine#38883)

* 78a68b9df Roll Skia from c64a10d525d1 to aa99fa0bdd31 (10 revisions) (flutter/engine#38886)

* 398af8c20 Roll Fuchsia Mac SDK from w0hr1ZMvYGJnWInwK... to nPbEqgIfE8TLOokQH... (flutter/engine#38889)

* 703502712 Roll Skia from aa99fa0bdd31 to 5a51583d09dd (2 revisions) (flutter/engine#38892)

* 2075cf3f8 Roll Fuchsia Mac SDK from nPbEqgIfE8TLOokQH... to SmEVvAFBC8oriPBou... (flutter/engine#38897)

* ef06ef602 Roll Skia from 5a51583d09dd to 34a14247bd4b (1 revision) (flutter/engine#38901)

* 0d374a919 Roll Skia from 34a14247bd4b to c727016cc09e (1 revision) (flutter/engine#38903)

* 31f2020d9 Roll Skia from c727016cc09e to 214411b09e92 (2 revisions) (flutter/engine#38907)

* a3006de27 Bump actions/setup-python from 4.4.0 to 4.5.0 (flutter/engine#38909)

* eacc09c47 Bump github/codeql-action from 2.1.37 to 2.1.38 (flutter/engine#38910)

* c33c11c93 Roll Skia from 214411b09e92 to 3704033dd106 (1 revision) (flutter/engine#38908)

* afa093dbe [Impeller] Map UVs correctly for TiledTextureContents (flutter/engine#38894)

* 46ea2b3cc Roll Fuchsia Mac SDK from SmEVvAFBC8oriPBou... to bZfX8KCoI3CfxEzsG... (flutter/engine#38912)

* aadb97112 Roll Skia from 3704033dd106 to 58e745419425 (4 revisions) (flutter/engine#38936)

* 24b21a5b4 Roll Fuchsia Mac SDK from bZfX8KCoI3CfxEzsG... to iwCuzAuMgsk75hlT0... (flutter/engine#38938)

* e65558087 Roll Skia from 58e745419425 to 6afb97022fa7 (1 revision) (flutter/engine#38943)

* 5932badce Revert "Revert "[web] Don't overwrite editing state with semantic updates (#38271)" (#38562)" (flutter/engine#38854)

* 928772eba [web] Reduce code size impact of fallback font data (flutter/engine#38787)

* 88a8d9a36 Roll Skia from 6afb97022fa7 to 8ea9b39f7213 (18 revisions) (flutter/engine#38952)

* 3a479434b Roll Skia from 8ea9b39f7213 to 69d8378097df (6 revisions) (flutter/engine#38953)

* 641623a51 wrap focus() in a post update callback (flutter/engine#38958)

* 7d71ee3bf Roll Fuchsia Mac SDK from iwCuzAuMgsk75hlT0... to -1N2YUML8GqNhK8YP... (flutter/engine#38960)

* d762142bb reland fix canvas drawLine bugs (flutter/engine#38949)

* 0d7953387 [Impeller] Fix back-to-back clip restoration (flutter/engine#38964)

* 19fe86ca9 Update scenario screenshots to iOS 16 (flutter/engine#38962)

* eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (flutter/engine#38822)

* 84a73d92f [Impeller Scene] Add SceneNodeValue for synchronously fetching loaded ipscenes (flutter/engine#38913)

* 7bff5dd11 Revert "Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (#38883)" (flutter/engine#38973)

* c52b29081 Roll Fuchsia Mac SDK from -1N2YUML8GqNhK8YP... to P5QcCJU8I71xVXuMT... (flutter/engine#38976)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Flutter Engine from f79030440948 to c52b290813bd (29 revisions) (f…
2211221 
…lutter#118720)

* 5aee7914c Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter/engine#38883)

* 78a68b9df Roll Skia from c64a10d525d1 to aa99fa0bdd31 (10 revisions) (flutter/engine#38886)

* 398af8c20 Roll Fuchsia Mac SDK from w0hr1ZMvYGJnWInwK... to nPbEqgIfE8TLOokQH... (flutter/engine#38889)

* 703502712 Roll Skia from aa99fa0bdd31 to 5a51583d09dd (2 revisions) (flutter/engine#38892)

* 2075cf3f8 Roll Fuchsia Mac SDK from nPbEqgIfE8TLOokQH... to SmEVvAFBC8oriPBou... (flutter/engine#38897)

* ef06ef602 Roll Skia from 5a51583d09dd to 34a14247bd4b (1 revision) (flutter/engine#38901)

* 0d374a919 Roll Skia from 34a14247bd4b to c727016cc09e (1 revision) (flutter/engine#38903)

* 31f2020d9 Roll Skia from c727016cc09e to 214411b09e92 (2 revisions) (flutter/engine#38907)

* a3006de27 Bump actions/setup-python from 4.4.0 to 4.5.0 (flutter/engine#38909)

* eacc09c47 Bump github/codeql-action from 2.1.37 to 2.1.38 (flutter/engine#38910)

* c33c11c93 Roll Skia from 214411b09e92 to 3704033dd106 (1 revision) (flutter/engine#38908)

* afa093dbe [Impeller] Map UVs correctly for TiledTextureContents (flutter/engine#38894)

* 46ea2b3cc Roll Fuchsia Mac SDK from SmEVvAFBC8oriPBou... to bZfX8KCoI3CfxEzsG... (flutter/engine#38912)

* aadb97112 Roll Skia from 3704033dd106 to 58e745419425 (4 revisions) (flutter/engine#38936)

* 24b21a5b4 Roll Fuchsia Mac SDK from bZfX8KCoI3CfxEzsG... to iwCuzAuMgsk75hlT0... (flutter/engine#38938)

* e65558087 Roll Skia from 58e745419425 to 6afb97022fa7 (1 revision) (flutter/engine#38943)

* 5932badce Revert "Revert "[web] Don't overwrite editing state with semantic updates (flutter#38271)" (flutter#38562)" (flutter/engine#38854)

* 928772eba [web] Reduce code size impact of fallback font data (flutter/engine#38787)

* 88a8d9a36 Roll Skia from 6afb97022fa7 to 8ea9b39f7213 (18 revisions) (flutter/engine#38952)

* 3a479434b Roll Skia from 8ea9b39f7213 to 69d8378097df (6 revisions) (flutter/engine#38953)

* 641623a51 wrap focus() in a post update callback (flutter/engine#38958)

* 7d71ee3bf Roll Fuchsia Mac SDK from iwCuzAuMgsk75hlT0... to -1N2YUML8GqNhK8YP... (flutter/engine#38960)

* d762142bb reland fix canvas drawLine bugs (flutter/engine#38949)

* 0d7953387 [Impeller] Fix back-to-back clip restoration (flutter/engine#38964)

* 19fe86ca9 Update scenario screenshots to iOS 16 (flutter/engine#38962)

* eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (flutter/engine#38822)

* 84a73d92f [Impeller Scene] Add SceneNodeValue for synchronously fetching loaded ipscenes (flutter/engine#38913)

* 7bff5dd11 Revert "Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter#38883)" (flutter/engine#38973)

* c52b29081 Roll Fuchsia Mac SDK from -1N2YUML8GqNhK8YP... to P5QcCJU8I71xVXuMT... (flutter/engine#38976)
@alanwutang11 alanwutang11 deleted the drawLine-bugs branch January 20, 2023 00:32
ricardoamador pushed a commit to ricardoamador/engine that referenced this pull request Jan 25, 2023
@alanwutang11
reland fix canvas drawLine bugs (flutter#38949)
151f3b9 
* fix canvas drawLine bugs

* removed unecessary params from pathToSvgElement

Co-authored-by: alanwutang11 <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yjbanov yjbanov yjbanov approved these changes

@mdebbar mdebbar Awaiting requested review from mdebbar

Assignees
No one assigned
Labels
platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem with Stack -> "ImageAsset + CustomPainter" with web-renderer html
2 participants
@alanwutang11 @yjbanov