Mirror before scaling in _AnimatedIconPainter #93312

Amir-P · 2021-11-09T10:11:35Z

This PR attempts to fix an issue in _AnimatedIconPainter causing wrong positioning of _AnimatedIconData when matchTextDirection set true in layouts with RTL directionality. This can be fixed by doing mirror before scaling the canvas.

Code to reproduce:

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

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Directionality(
        textDirection: TextDirection.rtl,
        child: Scaffold(
          appBar: AppBar(
            leading: IconButton(
              icon: AnimatedIcon(
                icon: AnimatedIcons.arrow_menu,
                progress: AlwaysStoppedAnimation(0),
              ),
              onPressed: () {},
            ),
          ),
        ),
      ),
    );
  }
}

Before:

After:

Issues this PR attempts to fix:

Fixes #60521

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

Piinks

Hi @Amir-P thanks for the contribution, and welcome!
Can you add another test that validates the positioning like from the example before/after images you shared?
Also, it looks like there are currently failing tests, can you take a look?

Amir-P · 2021-11-25T12:35:41Z

Hi @Amir-P thanks for the contribution, and welcome!

Can you add another test that validates the positioning like from the example before/after images you shared?

Also, it looks like there are currently failing tests, can you take a look?

Thanks! @Piinks
I've fixed the issue with private tests. ~~One of the checks (tree check) is still failing and I think that's not because of my changes.~~
Also I couldn't find a way to test the final position of drawn path. The position is determined by the transformations made to the canvas before drawing path and we have tests for that.
If you can guide me on writing tests for the position of path on canvas, please let me know.

Piinks

Also I couldn't find a way to test the final position of drawn path. The position is determined by the transformations made to the canvas before drawing path and we have tests for that.
If you can guide me on writing tests for the position of path on canvas, please let me know.

The type of test to add should take the code sample that illustrates your bug (above), and verify that the icon is placed in the right position now. This validates that it actually fixes the reported bug, and prevents it from regressing in the future.
Here is a similar test:

flutter/packages/flutter/test/material/app_bar_test.dart

Line 566 in 0e2cc64

    
           testWidgets('AppBar endDrawer icon has default size', (WidgetTester tester) async {

Instead of checking the size (tester.getSize), you would want to check the position, using tester.getCenter or tester.getTopLeft and validate that the icon is in the right place now.

Amir-P · 2021-12-24T09:29:47Z

Instead of checking the size (tester.getSize), you would want to check the position, using tester.getCenter or tester.getTopLeft and validate that the icon is in the right place now.

This change does not effect the widget and it's position. You can check the AnimatedIcon widget's position and size in both RTL and LTR direction and see there is no difference. It's happening in the painting level on canvas. You can validate the behavior using this small test (Before and after my commit):

testWidgets('Direction has no effect on AnimatedIcon widget position',
      (WidgetTester tester) async {
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.rtl,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect rtlRect = tester.getRect(find.byType(AnimatedIcon));
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.ltr,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect ltrRect = tester.getRect(find.byType(AnimatedIcon));
    expect(rtlRect, ltrRect);
  });

@Piinks

Piinks

testWidgets('Direction has no effect on AnimatedIcon widget position',
      (WidgetTester tester) async {
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.rtl,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect rtlRect = tester.getRect(find.byType(AnimatedIcon));
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.ltr,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect ltrRect = tester.getRect(find.byType(AnimatedIcon));
    expect(rtlRect, ltrRect);
  });

I couldn't find this test in the framework, can you add it? Thanks!

Also, we'll still need a test that validates the very real use case you have in your before and after image above. Otherwise we may regress in the future. The current test validates that you have changed the order of operations, not that the visual difference above has changed.

If checking the position does not validate this, then it sounds like a golden file test is more appropriate. Can you add one? Thanks!

Amir-P · 2022-01-10T20:20:16Z

Can you please check the latest commit? Some of the tests are failing but I'm not sure why because I didn't make any changes. Also I guess we need to upload those golden file images which I don't have access to. @Piinks

skia-gold · 2022-01-10T20:22:24Z

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

Piinks

If you visit https://flutter-gold.skia.org/cl/github/93312 you can see the images your test generated for each platform, I can approve them when this is ready to land. You may want to add a RepaintBoundary around the icon widget, right now the images are large white squares with a tiny icon in the top left corner. :)

It looks like the file with the golden file test needs a tag added at the top to pass analysis, the other failing checks should be fixed by updating your branch.

For more on golden file testing, check out this wiki page:
https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

Piinks · 2022-01-10T20:52:05Z

packages/flutter/test/material/animated_icons_test.dart

+  testWidgets('Inherited text direction overridden',
+      (WidgetTester tester) async {


Nit: Can you revert this style change?

Piinks · 2022-01-10T20:52:05Z

packages/flutter/test/material/animated_icons_test.dart

+  testWidgets('Direction has no effect on position of widget',
+      (WidgetTester tester) async {


Nit: This should be one line

You mean to remove the break line after ...widget', ?

Piinks · 2022-01-10T20:52:05Z

packages/flutter/test/material/animated_icons_test.dart

@@ -292,7 +334,7 @@ class PaintColorMatcher extends Matcher {

  @override
  Description describe(Description description) =>
-    description.add('color was not $expectedColor');
+      description.add('color was not $expectedColor');


Nit: revert this, these style changes don't match the style guide.

Amir-P · 2022-01-10T21:19:58Z

If you visit https://flutter-gold.skia.org/cl/github/93312 you can see the images your test generated for each platform, I can approve them when this is ready to land. You may want to add a RepaintBoundary around the icon widget, right now the images are large white squares with a tiny icon in the top left corner. :)

It looks like the file with the golden file test needs a tag added at the top to pass analysis, the other failing checks should be fixed by updating your branch.

For more on golden file testing, check out this wiki page: https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

Hope I got it right this time🤞🏻. @Piinks

skia-gold · 2022-01-10T21:22:24Z

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

Piinks · 2022-01-10T22:56:00Z

Coming along nicely! Thanks for the updates! It looks like you still need to update your PR with the latest from the master branch. :)

skia-gold · 2022-01-11T07:02:24Z

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

Amir-P · 2022-01-11T07:30:31Z

Coming along nicely! Thanks for the updates! It looks like you still need to update your PR with the latest from the master branch. :)

Done. @Piinks

flutter-dashboard · 2022-01-11T16:13:50Z

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.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #93312 at sha 0cb88c3

Piinks

Golden file images are approved, and this LGTM! Thank you!

Piinks · 2022-01-22T00:05:12Z

This will need a secondary review, I've reached out to the team for one. Meanwhile, can you take a look at the merge conflict here?

skia-gold · 2022-01-22T06:07:24Z

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

Amir-P · 2022-01-22T06:29:18Z

Done @Piinks .

Hixie · 2022-01-26T21:28:24Z

packages/flutter/test/material/animated_icons_test.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+@Tags(<String>['reduced-test-set'])


what does this do?

flutter.dev/go/reduce-ci-tests
https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter#reduced-test-set-tag

One of the velocity projects over the summer involved removing unit test runs on windows and mac machines. Just doing so though would remove our ability to provide a golden file test for every platform someone might be contributing to Flutter on, so we created a reduced test set for windows and mac. The analyzer will ensure now that all test files that contain a golden file test are included in the reduced test set.

Hixie

…ter#93312)

@NATHANIELCROSBY1

@NATHANIELCROSBY1 NATHANIELCROSBY1 create … 609e127 13 days ago Git stats 27,477 commits Files Type Name Latest commit message Commit time .github Bump github/codeql-action from 1.0.26 to 1.0.31 (flutter#97820) 14 days ago bin Roll Engine from 2a4709ae9fc4 to 0712096899f0 (1 revision) (flutter#9… 13 days ago dev Include -isysroot -arch and -miphoneos-version-min when creating dumm… 15 days ago examples Clean up the bindings APIs. (flutter#89451) 16 days ago packages Report progress on Dismissible update callback (flutter#95504) 14 days ago .ci.yaml Marks Linux_android opacity_peephole_fade_transition_text_perf__e2e_s… 15 days ago .cirrus.yml Pin dependencies in docker file. (flutter#97466) 18 days ago .gitattributes Add pre-stable support for create on Windows (flutter#51895) 2 years ago .gitignore Add macOS ephemeral to gitignore (flutter#96397) 20 days ago AUTHORS Mirror before scaling in _AnimatedIconPainter (flutter#93312) 24 days ago CODEOWNERS [codeowners] Remove *_builders.json ownership (flutter#91691) 4 months ago CODE_OF_CONDUCT.md Update CODE_OF_CONDUCT.md (flutter#94583) 3 months ago CONTRIBUTING.md Links How to contribute to Flutter YouTube video (flutter#96313) last month LICENSE License update (flutter#45373) 2 years ago PATENT_GRANT Rename patent file (flutter#38686) 3 years ago README.md Update README (flutter#97271) 24 days ago TESTOWNERS Add benchmarks to measure impact of alpha saveLayers in DisplayLists (f… 23 days ago analysis_options.yaml Enable no_leading_underscores_for_local_identifiers (flutter#96422) 29 days ago dartdoc_options.yaml Eliminate uses of pub executable in docs publishing and sample analys… 6 months ago flutter_console.bat License update (flutter#45373) 2 years ago git clone create 13 days ago README.md Flutter logo Build Status - Cirrus Discord badge Twitter handle Flutter is Google's SDK for crafting beautiful, fast user experiences for mobile, web, and desktop from a single codebase. Flutter works with existing code, is used by developers and organizations around the world, and is free and open source. Documentation Install Flutter Flutter documentation Development wiki Contributing to Flutter For announcements about new releases, follow the flutter-announce@googlegroups.com mailing list. Our documentation also tracks breaking changes across releases. Terms of service The Flutter tool may occasionally download resources from Google servers. By downloading or using the Flutter SDK you agree to the Google Terms of Service: https://policies.google.com/terms For example, when installed from GitHub (as opposed to from a prepackaged archive), the Flutter tool will download the Dart SDK from Google servers immediately when first run, as it is used to execute the flutter tool itself. This will also occur when Flutter is upgraded (e.g. by running the flutter upgrade command). About Flutter We think Flutter will help you create beautiful, fast apps, with a productive, extensible and open development model, whether you're targeting iOS or Android, web, Windows, macOS, Linux or embedding it as the UI toolkit for a platform of your choice. Beautiful user experiences We want to enable designers to deliver their full creative vision without being forced to water it down due to limitations of the underlying framework. Flutter's layered architecture gives you control over every pixel on the screen and its powerful compositing capabilities let you overlay and animate graphics, video, text, and controls without limitation. Flutter includes a full set of widgets that deliver pixel-perfect experiences whether you're building for iOS (Cupertino) or Android (Material), along with support for customizing or creating entirely new visual components. Reflectly hero image Fast results Flutter is fast. It's powered by the same hardware-accelerated 2D graphics library that underpins Chrome and Android: Skia. We architected Flutter to support glitch-free, jank-free graphics at the native speed of your device. Flutter code is powered by the world-class Dart platform, which enables compilation to 32-bit and 64-bit ARM machine code for iOS and Android, as well as JavaScript for the web and Intel x64 for desktop devices. Dart diagram Productive development Flutter offers stateful hot reload, allowing you to make changes to your code and see the results instantly without restarting your app or losing its state. Hot reload animation Extensible and open model Flutter works with any development tool (or none at all), and also includes editor plug-ins for both Visual Studio Code and IntelliJ / Android Studio. Flutter provides tens of thousands of packages to speed your development, regardless of your target platform. And accessing other native code is easy, with support for both FFI and platform-specific APIs. Flutter is a fully open-source project, and we welcome contributions. Information on how to get started can be found in our contributor guide.

flutter-dashboard bot added f: material design framework labels Nov 9, 2021

google-cla bot added the cla: yes label Nov 9, 2021

HansMuller requested a review from Piinks Nov 19, 2021

Piinks reviewed Nov 24, 2021

View changes

Amir-P force-pushed the fix_60521 branch from 26a9c12 to f0c7e3c Nov 25, 2021

Amir-P requested a review from Piinks Nov 26, 2021

Piinks requested changes Dec 23, 2021

View changes

Piinks reviewed Jan 4, 2022

View changes

Amir-P requested a review from Piinks Jan 10, 2022

Piinks reviewed Jan 10, 2022

View changes

Amir-P requested a review from Piinks Jan 10, 2022

Amir-P added 4 commits Jan 11, 2022

Fixed flutter#60521

7add09b

Update animated_icons_private_test.dart.tmpl

18a364b

Added golden file tests

fd62ff3

Wrapped icons in RepaintBoundary, fixed style issues

0cb88c3

Amir-P force-pushed the fix_60521 branch from 6f3f656 to 0cb88c3 Jan 11, 2022

flutter-dashboard bot added the will affect goldens label Jan 11, 2022

Piinks approved these changes Jan 22, 2022

View changes

Merge branch 'master' into fix_60521

f1555e3

Hixie reviewed Jan 26, 2022

View changes

Hixie approved these changes Jan 26, 2022

View changes

Hixie added the waiting for tree to go green label Jan 26, 2022

fluttergithubbot merged commit 42a8122 into flutter:master Jan 26, 2022
65 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this issue Jan 26, 2022

42a812231 Mirror before scaling in _AnimatedIconPainter (flutter/flut…

c839e23

…ter#93312)

engine-flutter-autoroll mentioned this pull request Jan 26, 2022

Roll Flutter from 32122480b5cf to f15dd780d4c5 (2 revisions) flutter/packages#564

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this issue Feb 4, 2022

42a812231 Mirror before scaling in _AnimatedIconPainter (flutter/flut…

9014d5d

…ter#93312)

LeonardoRosaa added a commit to LeonardoRosaa/flutter that referenced this issue Feb 7, 2022

Mirror before scaling in _AnimatedIconPainter (flutter#93312)

c443353

NATHANIELCROSBY1 mentioned this pull request Feb 19, 2022

tyetech NATHANIELCROSBY1/flutter#1

Open

8 tasks

clocksmith added a commit to clocksmith/flutter that referenced this issue Mar 8, 2022

Mirror before scaling in _AnimatedIconPainter (flutter#93312)

cc5046d

flutter / flutter Public

Mirror before scaling in _AnimatedIconPainter #93312

Mirror before scaling in _AnimatedIconPainter #93312

Amir-P commented Nov 9, 2021 •

edited by Piinks

Piinks left a comment

Amir-P commented Nov 25, 2021 •

edited

Piinks left a comment

Amir-P commented Dec 24, 2021

Piinks left a comment

Amir-P commented Jan 10, 2022 •

edited

skia-gold commented Jan 10, 2022

Piinks left a comment

Piinks Jan 10, 2022

Piinks Jan 10, 2022

Amir-P Jan 10, 2022

Piinks Jan 10, 2022

Piinks Jan 10, 2022

Amir-P commented Jan 10, 2022

skia-gold commented Jan 10, 2022

Piinks commented Jan 10, 2022

skia-gold commented Jan 11, 2022

Amir-P commented Jan 11, 2022

flutter-dashboard bot commented Jan 11, 2022

Piinks left a comment

Piinks commented Jan 22, 2022

skia-gold commented Jan 22, 2022

Amir-P commented Jan 22, 2022

Hixie Jan 26, 2022

Piinks Jan 26, 2022

Hixie Jan 26, 2022

Hixie left a comment

		testWidgets('Inherited text direction overridden',
		(WidgetTester tester) async {

		testWidgets('Direction has no effect on position of widget',
		(WidgetTester tester) async {

flutter / flutter Public

Mirror before scaling in _AnimatedIconPainter #93312

Mirror before scaling in _AnimatedIconPainter #93312

Conversation

Amir-P commented Nov 9, 2021 • edited by Piinks

Code to reproduce:

Issues this PR attempts to fix:

Pre-launch Checklist

Piinks left a comment

Amir-P commented Nov 25, 2021 • edited

Piinks left a comment

Amir-P commented Dec 24, 2021

Piinks left a comment

Amir-P commented Jan 10, 2022 • edited

skia-gold commented Jan 10, 2022

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amir-P commented Jan 10, 2022

skia-gold commented Jan 10, 2022

Piinks commented Jan 10, 2022

skia-gold commented Jan 11, 2022

Amir-P commented Jan 11, 2022

flutter-dashboard bot commented Jan 11, 2022

Piinks left a comment

Piinks commented Jan 22, 2022

skia-gold commented Jan 22, 2022

Amir-P commented Jan 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie left a comment

Amir-P commented Nov 9, 2021 •

edited by Piinks

Amir-P commented Nov 25, 2021 •

edited

Amir-P commented Jan 10, 2022 •

edited