engine
engine copied to clipboard
fix: mask disappeared when having nested mask filter on Flutter web HTML
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.
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.
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
Golden file changes are available for triage from new commit, Click here to view.
Changes reported for pull request #45166 at sha f22f02e7938e1292f532055b7d790791f633dacd
Any updates here? :)
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.
Hi, just seeing this now. I am looking into image filters right now and will look into this tomorrow.
Hello @Kingtous, do you plan to rebase this PR on the latest main commit to fix the goldens? Thanks
Hello @Kingtous, do you plan to rebase this PR on the latest
maincommit to fix the goldens? Thanks
Sure, I'll rebase this PR to the latest main commit these days. Thanks for reviewing this PR :)
Golden file changes are available for triage from new commit, Click here to view.
Changes reported for pull request #45166 at sha d4c7c4e4fdf9978d872fcc4b9ca5e9c0d6476e03
Hello @Kingtous, do you plan to rebase this PR on the latest
maincommit to fix the goldens? Thanks
Done.
Golden file changes are available for triage from new commit, Click here to view.
Changes reported for pull request #45166 at sha 3528c73412d21e7ee1a51798019793e209af5273
I'm updating the branch to determine up-to-date golden file changes
Golden file changes are available for triage from new commit, Click here to view.
Changes reported for pull request #45166 at sha 9f44c8fa9a37f2611e23e45cd7ce92c2d7d81d6d
Golden file changes are available for triage from new commit, Click here to view.
Changes reported for pull request #45166 at sha a29b7b4e18431079c7fa029826195f37d6fe0703
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.
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
After this change
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.