Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mask disappeared when having nested mask filter on Flutter web HTML #45166

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

Kingtous
Copy link
Contributor

@Kingtous Kingtous commented Aug 28, 2023

Hi from Dora team, which powers web developers to build their 3d websites in just a few seconds.

This PR fixes: flutter/flutter#133443, related: flutter/flutter#58546

The original codes attempts to cache the css string but it causes bugs when encountering nested the same mask filter blur. We should also set filter properties when currentFilter == incoming mask filter using the cached css string, not just ignore it.

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
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.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Aug 28, 2023
@Kingtous Kingtous force-pushed the fix/nested_same_mask_filter branch from d2376e4 to efbd4ba Compare August 28, 2023 10:12
@Kingtous Kingtous force-pushed the fix/nested_same_mask_filter branch from c3b0ac6 to 9c48c87 Compare August 29, 2023 01:27
@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 #45166 at sha 9c48c87

@flutter-dashboard
Copy link

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

Changes reported for pull request #45166 at sha f22f02e

@Kingtous
Copy link
Contributor Author

Kingtous commented Sep 6, 2023

Any updates here? :)

@flutter-dashboard
Copy link

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.

@harryterkelsen harryterkelsen self-requested a review December 1, 2023 00:14
@harryterkelsen
Copy link
Contributor

Hi, just seeing this now. I am looking into image filters right now and will look into this tomorrow.

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. Sorry for the delay in review, I am making it a priority going forward to review third-party contributions.

Please rebase on top of main so the golden tests aren't polluted with all the new goldens that have been added since this PR was created.

Again, thanks so much! External contributors like you make Flutter great!

@harryterkelsen
Copy link
Contributor

Hello @Kingtous, do you plan to rebase this PR on the latest main commit to fix the goldens? Thanks

@Kingtous
Copy link
Contributor Author

Hello @Kingtous, do you plan to rebase this PR on the latest main commit to fix the goldens? Thanks

Sure, I'll rebase this PR to the latest main commit these days. Thanks for reviewing this PR :)

@Kingtous Kingtous force-pushed the fix/nested_same_mask_filter branch from f22f02e to d4c7c4e Compare February 15, 2024 15:51
@flutter-dashboard
Copy link

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

Changes reported for pull request #45166 at sha d4c7c4e

@Kingtous Kingtous force-pushed the fix/nested_same_mask_filter branch from d4c7c4e to 3528c73 Compare February 16, 2024 11:34
@Kingtous
Copy link
Contributor Author

Hello @Kingtous, do you plan to rebase this PR on the latest main commit to fix the goldens? Thanks

Done.

@flutter-dashboard
Copy link

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

Changes reported for pull request #45166 at sha 3528c73

@harryterkelsen
Copy link
Contributor

I'm updating the branch to determine up-to-date golden file changes

@flutter-dashboard
Copy link

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

Changes reported for pull request #45166 at sha 9f44c8f

@flutter-dashboard
Copy link

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

Changes reported for pull request #45166 at sha a29b7b4

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
Copy link
Contributor

auto-submit bot commented Jul 3, 2024

auto label is removed for flutter/engine/45166, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

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

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
@auto-submit auto-submit bot merged commit c90aa9a into flutter:main Jul 3, 2024
32 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 4, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 4, 2024
…151293)

flutter/engine@4190543...8e2d05f

2024-07-04 flar@google.com [Impeller] Re-enable fast blur path for elliptical rrects (flutter/engine#53704)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 9fd1dc779589 to d5f8dde714e4 (2 revisions) (flutter/engine#53721)
2024-07-03 yjbanov@google.com [web] ignore pointer events on plain text spans (flutter/engine#53694)
2024-07-03 mdebbar@google.com Add Semantics Property `linkUrl` (flutter/engine#53507)
2024-07-03 chinmaygarde@google.com [Embedder] Document incorrectly named field in FlutterOpenGLFramebuffer. (flutter/engine#53720)
2024-07-03 chinmaygarde@google.com [Impeller] Make storage sizes typed. (flutter/engine#53700)
2024-07-03 matanlurey@users.noreply.github.com Convert `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53645)
2024-07-03 matanlurey@users.noreply.github.com Move `//third_party/android_embedding_dependencies` to `//flutter/third_party`. (flutter/engine#53587)
2024-07-03 chinmaygarde@google.com [Impeller] Document how to debug/profile OpenGL ES on macOS. (flutter/engine#53671)
2024-07-03 kingtous@qq.com [Flutter Web(HTML)] fix: shader mask is painted incorrectly on shared offscreen canvas (flutter/engine#44998)
2024-07-03 kingtous@qq.com fix: mask disappeared when having nested mask filter on Flutter web HTML (flutter/engine#45166)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 86ee8cc61508 to 9fd1dc779589 (3 revisions) (flutter/engine#53715)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from c14cce59222b to 86ee8cc61508 (1 revision) (flutter/engine#53713)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jiahaog
Copy link
Member

jiahaog commented Jul 4, 2024

Reason for revert: This commit seems to cause the debug banner of a material app to be unnecessarily blurred. For example, one of the tests runs the following flutter app:

Code
import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData(useMaterial3: false),
      title: 'Flutter Demo',
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key? key, required this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  int _counter = 0;

  void _incrementCounter() {
    setState(() {
      _counter++;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title, style: TextStyle(fontFamily: 'ProductSans')),
      ),
      body: Center(
        child: Text(
          'Button tapped $_counter time${_counter == 1 ? '' : 's'}.',
          key: Key('CountText'),
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ),
    );
  }
}

Screenshots:

Before this change

image

After this change

image

Observe that the debug banner on the top right is now blurred. This test runs on Chrome Linux v126, and the unexpected blurs can be seen in both DDC and Dart2js mode with the html renderer.

This can also be reproduced with a fresh flutter app in chrome v126 with the following command on linux (didn't check other platforms), with a Flutter checkout at flutter/flutter@fa70e61. It does not reproduce with the parent commit, nor with the --web-renderer argument ommited.

flutter run web -d chrome --web-renderer html

Googlers, see b/351082848 for more details.

@jiahaog jiahaog added the revert Label used to revert changes in a closed and merged pull request. label Jul 4, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 4, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jul 4, 2024
auto-submit bot added a commit that referenced this pull request Jul 4, 2024
…ter web HTML (#45166)" (#53725)

Reverts: #45166
Initiated by: jiahaog
Reason for reverting: This commit seems to cause the debug banner of a material app to be unnecessarily blurred. For example, one of the tests runs the following flutter app:

<details>

<summary>Code</summary>

```dart
import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @OverRide
  Widget build(BuildContext context) {
    return Materia
Original PR Author: Kingtous

Reviewed By: {yjbanov, harryterkelsen}

This change reverts the following previous change:
Hi from [Dora team](https://www.dora.run/), which powers web developers to build their 3d websites in just a few seconds.

This PR fixes: flutter/flutter#133443, related: flutter/flutter#58546

The original codes attempts to cache the css string but it causes bugs when encountering nested the same mask filter blur. We should also set `filter` properties when currentFilter == incoming mask filter using the cached css string, not just ignore it. 

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
TahaTesser pushed a commit to TahaTesser/flutter that referenced this pull request Jul 8, 2024
…lutter#151293)

flutter/engine@4190543...8e2d05f

2024-07-04 flar@google.com [Impeller] Re-enable fast blur path for elliptical rrects (flutter/engine#53704)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 9fd1dc779589 to d5f8dde714e4 (2 revisions) (flutter/engine#53721)
2024-07-03 yjbanov@google.com [web] ignore pointer events on plain text spans (flutter/engine#53694)
2024-07-03 mdebbar@google.com Add Semantics Property `linkUrl` (flutter/engine#53507)
2024-07-03 chinmaygarde@google.com [Embedder] Document incorrectly named field in FlutterOpenGLFramebuffer. (flutter/engine#53720)
2024-07-03 chinmaygarde@google.com [Impeller] Make storage sizes typed. (flutter/engine#53700)
2024-07-03 matanlurey@users.noreply.github.com Convert `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53645)
2024-07-03 matanlurey@users.noreply.github.com Move `//third_party/android_embedding_dependencies` to `//flutter/third_party`. (flutter/engine#53587)
2024-07-03 chinmaygarde@google.com [Impeller] Document how to debug/profile OpenGL ES on macOS. (flutter/engine#53671)
2024-07-03 kingtous@qq.com [Flutter Web(HTML)] fix: shader mask is painted incorrectly on shared offscreen canvas (flutter/engine#44998)
2024-07-03 kingtous@qq.com fix: mask disappeared when having nested mask filter on Flutter web HTML (flutter/engine#45166)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 86ee8cc61508 to 9fd1dc779589 (3 revisions) (flutter/engine#53715)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from c14cce59222b to 86ee8cc61508 (1 revision) (flutter/engine#53713)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
…lutter#151293)

flutter/engine@4190543...8e2d05f

2024-07-04 flar@google.com [Impeller] Re-enable fast blur path for elliptical rrects (flutter/engine#53704)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 9fd1dc779589 to d5f8dde714e4 (2 revisions) (flutter/engine#53721)
2024-07-03 yjbanov@google.com [web] ignore pointer events on plain text spans (flutter/engine#53694)
2024-07-03 mdebbar@google.com Add Semantics Property `linkUrl` (flutter/engine#53507)
2024-07-03 chinmaygarde@google.com [Embedder] Document incorrectly named field in FlutterOpenGLFramebuffer. (flutter/engine#53720)
2024-07-03 chinmaygarde@google.com [Impeller] Make storage sizes typed. (flutter/engine#53700)
2024-07-03 matanlurey@users.noreply.github.com Convert `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53645)
2024-07-03 matanlurey@users.noreply.github.com Move `//third_party/android_embedding_dependencies` to `//flutter/third_party`. (flutter/engine#53587)
2024-07-03 chinmaygarde@google.com [Impeller] Document how to debug/profile OpenGL ES on macOS. (flutter/engine#53671)
2024-07-03 kingtous@qq.com [Flutter Web(HTML)] fix: shader mask is painted incorrectly on shared offscreen canvas (flutter/engine#44998)
2024-07-03 kingtous@qq.com fix: mask disappeared when having nested mask filter on Flutter web HTML (flutter/engine#45166)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 86ee8cc61508 to 9fd1dc779589 (3 revisions) (flutter/engine#53715)
2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from c14cce59222b to 86ee8cc61508 (1 revision) (flutter/engine#53713)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flutter Web] Incorrect render result when having the nested same mask-filter
4 participants