engine icon indicating copy to clipboard operation
engine copied to clipboard

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

Open Kingtous opened this issue 2 years ago • 11 comments
trafficstars

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

This PR fixes: https://github.com/flutter/flutter/issues/133443, related: https://github.com/flutter/flutter/issues/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

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the CLA.
  • [x] All existing and new tests are passing.

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

Kingtous avatar Aug 28 '23 10:08 Kingtous

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.

flutter-dashboard[bot] avatar Aug 28 '23 10:08 flutter-dashboard[bot]

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 9c48c877b31c40769e56997a00b0692687d7bd08

flutter-dashboard[bot] avatar Aug 29 '23 02:08 flutter-dashboard[bot]

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

Changes reported for pull request #45166 at sha f22f02e7938e1292f532055b7d790791f633dacd

flutter-dashboard[bot] avatar Aug 29 '23 03:08 flutter-dashboard[bot]

Any updates here? :)

Kingtous avatar Sep 06 '23 03:09 Kingtous

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.

flutter-dashboard[bot] avatar Sep 26 '23 03:09 flutter-dashboard[bot]

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

harryterkelsen avatar Dec 01 '23 00:12 harryterkelsen

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

harryterkelsen avatar Feb 13 '24 23:02 harryterkelsen

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 avatar Feb 14 '24 14:02 Kingtous

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

Changes reported for pull request #45166 at sha d4c7c4e4fdf9978d872fcc4b9ca5e9c0d6476e03

flutter-dashboard[bot] avatar Feb 15 '24 16:02 flutter-dashboard[bot]

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

Done.

Kingtous avatar Feb 16 '24 11:02 Kingtous

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

Changes reported for pull request #45166 at sha 3528c73412d21e7ee1a51798019793e209af5273

flutter-dashboard[bot] avatar Feb 16 '24 12:02 flutter-dashboard[bot]

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

harryterkelsen avatar Apr 23 '24 21:04 harryterkelsen

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

Changes reported for pull request #45166 at sha 9f44c8fa9a37f2611e23e45cd7ce92c2d7d81d6d

flutter-dashboard[bot] avatar Apr 23 '24 22:04 flutter-dashboard[bot]

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

Changes reported for pull request #45166 at sha a29b7b4e18431079c7fa029826195f37d6fe0703

flutter-dashboard[bot] avatar Jul 02 '24 23:07 flutter-dashboard[bot]

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.

auto-submit[bot] avatar Jul 03 '24 17:07 auto-submit[bot]

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 https://github.com/flutter/flutter/commit/fa70e610cc244750b07321c466385d821533ee68. 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 avatar Jul 04 '24 06:07 jiahaog