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

Fix html gradient rendering (https://github.com/flutter/flutter/issues/97762) #31355

Merged
merged 4 commits into from
Feb 10, 2022

Conversation

eyebrowsoffire
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire commented Feb 9, 2022

Fixes flutter/flutter#97762

There are actually three separate issues here to fix:

  1. UV coordinates for the repeated gradient are messed up. The reason for this is that
    there is a transformation to the gradient matrix that we were for some reason
    skipping when the tile mode was "repeated". We need to apply that transformation
    in the "repeated" tile mode as well.
  2. Coloration of some gradients is wrong when using a color with an alpha channel. The
    reason this is occurring is because the transferToImageBitmap doesn't properly
    preserve the alpha channel. The fix is to use the fallback path if the gradient
    contains a non-opaque color.
  3. Sweep gradient wasn't drawing properly at all. This is because if there was no
    transformation matrix passed into the shader, we didn't set the gradient matrix
    uniform at all. We should set it to the identity matrix if there is none specified.

There are actually three separate issues here to fix:
1) UV coordinates for the repeated gradient are messed up. The reason for this is that
   there is a transformation to the gradient matrix that we were for some reason
   skipping when the tile mode was "repeated". We need to apply that transformation
   in the "repeated" tile mode as well.
2) Coloration of some gradients is wrong when using a color with an alpha channel. The
   reason this is occurring is because the transferToImageBitmap doesn't properly
   preserve the alpha channel. The fix is to use the fallback path if the gradient
   contains a non-opaque color.
3) Sweep gradient wasn't drawing properly at all. This is because if there was no
   transformation matrix passed into the shader, we didn't set the gradient matrix
   uniform at all. We should set it to the identity matrix if there is none specified.
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Feb 9, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@eyebrowsoffire
Copy link
Contributor Author

Linear before change:
linear_before

Linear after change:
linear_after

Sweep before change:
sweep_before

Sweep after change:
sweep_after

It should be noted that these are all consistent with CanvasKit now.

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 1.
View them at https://flutter-engine-gold.skia.org/cl/github/31355

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/31355

@flutter-dashboard
Copy link

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.

Changes reported for pull request #31355 at sha 985bee6

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.

An example screenshot test for this (in fact, we should probably just add more tests to that save file) is https://github.com/flutter/engine/blob/main/lib/web_ui/test/html/shaders/gradient_golden_test.dart

// When using OffscreenCanvas and transferToImageBitmap is supported by
// browser create ImageBitmap otherwise use more expensive canvas
// allocation.
// allocation. However, transferToImageBitmap does not properly preserve
// the alpha channel, so only use it if the pattern is opaque.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is because on the following line we should also specify the alpha parameter:

: glContext = canvas.getContext('webgl2', <String, dynamic>{'premultipliedAlpha': false})!,

According to MDN docs: "alpha: Boolean that indicates if the canvas contains an alpha channel. If set to false, the browser now knows that the backdrop is always opaque, which can speed up drawing of transparent content and images."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually tried this already! It did nothing unfortunately. I believe alpha is true by default.

@skia-gold
Copy link

Gold has detected about 5 new digest(s) on patchset 3.
View them at https://flutter-engine-gold.skia.org/cl/github/31355

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #31355 at sha d936d50

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

@skia-gold
Copy link

Gold has detected about 12 new digest(s) on patchset 4.
View them at https://flutter-engine-gold.skia.org/cl/github/31355

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #31355 at sha c217f27

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@yjbanov yjbanov added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 10, 2022
@fluttergithubbot fluttergithubbot merged commit 3d629c5 into flutter:main Feb 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Feb 11, 2022
* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352)

* ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291)

* 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191)

* 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353)

* 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356)

* 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357)

* b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360)

* 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362)

* 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351)

* 2cc3abb Manual roll of ICU (flutter/engine#31132)

* c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364)

* fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365)

* 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571)

* 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366)

* 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334)

* 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368)

* 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369)

* b04ed63 Change link to felt documentation (flutter/engine#31312)

* 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370)

* 90effff Revert "Add trackpad gesture PointerData types (#28571)" (flutter/engine#31375)

* 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361)

* 3d629c5 Fix html gradient rendering (#97762) (flutter/engine#31355)

* 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246)

* 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372)

* 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373)

* 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374)

* 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377)

* f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376)

* d36d25f TextEditingDelta Support for the Web (flutter/engine#28527)

* aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378)

* 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919)

* 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380)

* 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381)

* 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383)

* 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382)

* d1164c1 Move recipes to repository folders. (flutter/engine#31367)

* e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384)

* 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605)

* 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397)

* 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398)

* ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352)

* ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291)

* 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191)

* 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353)

* 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356)

* 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357)

* b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360)

* 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362)

* 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351)

* 2cc3abb Manual roll of ICU (flutter/engine#31132)

* c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364)

* fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365)

* 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571)

* 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366)

* 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334)

* 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368)

* 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369)

* b04ed63 Change link to felt documentation (flutter/engine#31312)

* 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370)

* 90effff Revert "Add trackpad gesture PointerData types (flutter#28571)" (flutter/engine#31375)

* 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361)

* 3d629c5 Fix html gradient rendering (flutter#97762) (flutter/engine#31355)

* 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246)

* 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372)

* 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373)

* 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374)

* 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377)

* f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376)

* d36d25f TextEditingDelta Support for the Web (flutter/engine#28527)

* aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378)

* 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919)

* 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380)

* 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381)

* 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383)

* 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382)

* d1164c1 Move recipes to repository folders. (flutter/engine#31367)

* e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384)

* 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605)

* 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397)

* 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398)

* ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs tests platform-web Code specifically for the web engine waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web][html] Gradients with TileMode.mirror or TileMode.repeated are not rendered properly if Alpha is 0
5 participants