Update visual style of CupertinoSwitch to match iOS 13 #36087

brandondiamond · 2019-07-12T18:57:02Z

Description

Updates the CupertinoSwitch widget to reflect the revised design of switches in iOS 13. In particular, the border no longer animates in response to user interaction. Instead, the background fades between colors.

After:
https://i.imgur.com/dMV7T5G.gif (slow 5x)
https://i.imgur.com/arKDoNk.gif (normal)

Before:
https://i.imgur.com/SxDYis4.gif (slow 5x)
https://i.imgur.com/k7HCNJU.gif (normal)

Related Issues

Issue #33797

Tests

Added a test to verify golden images of the slider in on and off states.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-07-12T18:57:04Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

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

/cc @dnfield

LongCatIsLooong · 2019-07-16T00:28:42Z

Hi @brandondiamond thank you for contributing, the PR looks awesome! However tests are mandatory for PRs like this. Could you add tests to ensure it works as expected?

In case you want to add golden tests: https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

brandondiamond · 2019-07-16T19:32:37Z

Thanks so much for the quick review! I'll get started on those golden tests right away. Given that this is a visual change, are there any additional tests (beyond the golden tests) that you would suggest?

brandondiamond · 2019-07-16T22:07:09Z

Added GIFs of before/after:

After:
https://i.imgur.com/dMV7T5G.gif (slow 5x)
https://i.imgur.com/arKDoNk.gif (normal)

Before:
https://i.imgur.com/SxDYis4.gif (slow 5x)
https://i.imgur.com/k7HCNJU.gif (normal)

LongCatIsLooong · 2019-07-16T23:18:09Z

@brandondiamond thanks for the GIFs! Adding golden images when the switch is inactive/active/somewhere in between should be good enough.

justinmc

Thanks for the PR, this is great!

+1 for tests. I think that one golden image for on and one for off should be fine.

One very small visual nitpick. I think that our shadow extends below the switch track when it shouldn't anymore.

	This Branch	iOS 13
On
Off

brandondiamond · 2019-07-17T18:07:48Z

Thanks Justin! I added a clip to ensure the thumb shadow doesn't exceed the track. I know clipping can be expensive so wanted to verify that it'd be okay in this case.

I also wanted to be mindful of antialiasing (since the clip is curved), so I used saveLayer to group the thumb drawing commands together. There's an additional save() and restore() around the clip because I wasn't certain whether there's an automatic save/restore around each paint() -- particularly for render objects that don't mark repaint boundaries; I don't want subsequent drawing operations to be affected by the clip.

I'm very grateful for the opportunity to contribute -- thanks again for reviewing :)

brandondiamond · 2019-07-17T20:20:03Z

Added golden tests and generated goldens. I followed the instructions here but ran into permission issues when attempting to push my golden assets commit to the repo.

What's the correct way to push a change to the goldens repo without special permissions?

justinmc · 2019-07-17T22:01:53Z

Thanks! I tried running this again and now that it's clipped it looks perfect to my eye.

Regarding performance and antialiasing, I'm going to track down someone that can confirm.

Regarding permissions on the goldens repo, I believe you'll have to fork that repo and open a PR like you did with this PR. I know we tightened up this process recently so I could be wrong, but that's how I did it a few months ago. If you post back with a link to the goldens PR, I can review it really quickly though.

brandondiamond · 2019-07-17T22:51:54Z

Awesome, thanks Justin :)

Here's the goldens PR:
flutter/goldens#40

Once that goes through, I'll update this PR to reference the commit that includes the new goldens.

packages/flutter/test/cupertino/switch_test.dart

packages/flutter/lib/src/cupertino/switch.dart

packages/flutter/test/cupertino/switch_test.dart

codecov · 2019-07-17T23:54:58Z

Codecov Report

Merging #36087 into master will decrease coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36087      +/-   ##
==========================================
- Coverage   55.74%   55.36%   -0.38%     
==========================================
  Files         190      188       -2     
  Lines       17585    17345     -240     
==========================================
- Hits         9802     9603     -199     
+ Misses       7783     7742      -41

Flag	Coverage Δ
#flutter_tool	`55.36% <0%> (-0.38%)`	⬇️

Impacted Files	Coverage Δ
...ages/flutter_tools/lib/src/commands/build_web.dart	`33.33% <0%> (-28.21%)`	⬇️
...ackages/flutter_tools/lib/src/reporting/usage.dart	`68.25% <0%> (-17.67%)`	⬇️
packages/flutter_tools/lib/src/web/workflow.dart	`77.77% <0%> (-14.53%)`	⬇️
...ages/flutter_tools/lib/src/commands/build_aot.dart	`22.85% <0%> (-12.86%)`	⬇️
packages/flutter_tools/lib/src/desktop.dart	`78.57% <0%> (-8.39%)`	⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart	`37.62% <0%> (-7.56%)`	⬇️
...utter_tools/lib/src/build_system/targets/dart.dart	`84.61% <0%> (-6.53%)`	⬇️
...s/flutter_tools/lib/src/commands/build_bundle.dart	`93.87% <0%> (-4.24%)`	⬇️
packages/flutter_tools/lib/src/artifacts.dart	`64.66% <0%> (-3.57%)`	⬇️
packages/flutter_tools/lib/src/run_hot.dart	`45.83% <0%> (-2.86%)`	⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebdc2cf...3ceef68. Read the comment docs.

packages/flutter/lib/src/cupertino/switch.dart

brandondiamond · 2019-07-18T17:19:05Z

Updated the tests, generated new goldens (ready to go in flutter/goldens#40 -- I'll update the golden hash here once that is merged), and switched to using the PaintingContext to push a clip.

packages/flutter/lib/src/cupertino/switch.dart

Co-Authored-By: LongCatIsLooong <[email protected]>

brandondiamond · 2019-07-18T18:11:51Z

Thanks for the review! I'm going to read the style guide today to make sure I don't repeat these formatting mistakes [too often :)]

…into cupertino_switch

brandondiamond · 2019-07-18T21:17:16Z

Goldens merged and bumped the golden version! Should be good to go.

LongCatIsLooong · 2019-07-18T22:29:28Z

packages/flutter/lib/src/cupertino/switch.dart

-    ));
+    );
+
+    context.pushClipRRect(needsCompositing, Offset.zero, thumbBounds, trackRRect, (PaintContext innerContext, Offset offset) {


PaintContext => PaintingContext, sorry for the wrong suggestion.

brandondiamond · 2019-07-19T00:05:19Z

No worries! I've made that mistake many times

packages/flutter/lib/src/cupertino/switch.dart

Co-Authored-By: LongCatIsLooong <[email protected]>

packages/flutter/test/cupertino/switch_test.dart

Co-Authored-By: LongCatIsLooong <[email protected]>

Piinks · 2019-07-22T15:18:45Z

Currently blocked by #36690, which will resolve the golden sync issues that are causing the tests to fail here.

brandondiamond · 2019-07-22T16:38:39Z

Thanks for looking into this. Please let me know if there is anything I can do to help.

Piinks

#36690 has landed. You may need to update your branch by pulling down the changes from master.

brandondiamond · 2019-07-23T17:35:19Z

@Piinks Thanks! What's the best way to do this without messing up commit history?

Piinks · 2019-07-23T17:40:35Z

You should be able to do a
git fetch upstream
and then
git pull upstream master
This will create a commit that merges updates from master into your branch, which you can then push here. :)

…itch

brandondiamond · 2019-07-23T17:52:45Z

Thanks so much! Just pushed -- hopefully all looks okay :)

Piinks

LGTM! Thank you for the contribution! 🎉

justinmc

LGTM. Thanks for this! I'll merge soon.

brandondiamond added 2 commits July 12, 2019 14:48

Update visual style of cupertino switch to match ios13

8b79cc0

Remove math import, as it's no longer necessary.

65bbd4f

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Jul 12, 2019

googlebot added the cla: yes label Jul 12, 2019

brandondiamond mentioned this pull request Jul 12, 2019

[iOS 13] cupertino switch is different #33797

Closed

justinmc reviewed Jul 17, 2019

View reviewed changes

Add clip to prevent thumb from exceeding track.

b5e8483

Add golden tests for restyled switch

60df58c

brandondiamond mentioned this pull request Jul 17, 2019

Add golden images for revised cupertino switch. flutter/goldens#40

Merged

LongCatIsLooong reviewed Jul 17, 2019

View reviewed changes

packages/flutter/test/cupertino/switch_test.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Jul 17, 2019

View reviewed changes

packages/flutter/lib/src/cupertino/switch.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Jul 17, 2019

View reviewed changes

packages/flutter/test/cupertino/switch_test.dart Show resolved Hide resolved

Add intermediate test and use different clipping mechanism.

f7eb4cb

brandondiamond commented Jul 18, 2019

View reviewed changes

packages/flutter/lib/src/cupertino/switch.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Jul 18, 2019

View reviewed changes

packages/flutter/lib/src/cupertino/switch.dart Outdated Show resolved Hide resolved

brandondiamond and others added 2 commits July 18, 2019 13:51

Add missing type annotation

884ca8d

Add types and names to paint closure.

7e3ce5b

Co-Authored-By: LongCatIsLooong <[email protected]>

LongCatIsLooong pushed a commit to flutter/goldens that referenced this pull request Jul 18, 2019

Goldens for flutter/flutter#36087 (#40)

d35b254

brandondiamond added 2 commits July 18, 2019 17:12

Bump goldens version to include new assets

21e8989

Merge branch 'cupertino_switch' of github.com:brandondiamond/flutter …

6c64fca

…into cupertino_switch

LongCatIsLooong reviewed Jul 18, 2019

View reviewed changes

PaintContext => PainterContext (whoops!)

751850d

LongCatIsLooong reviewed Jul 19, 2019

View reviewed changes

packages/flutter/lib/src/cupertino/switch.dart Outdated Show resolved Hide resolved

Fix typo (let's pretend this didn't happen)

609d8ef

Co-Authored-By: LongCatIsLooong <[email protected]>

LongCatIsLooong reviewed Jul 19, 2019

View reviewed changes

packages/flutter/test/cupertino/switch_test.dart Outdated Show resolved Hide resolved

Remove trailing space

d9895ae

Co-Authored-By: LongCatIsLooong <[email protected]>

This was referenced Jul 19, 2019

Do Not Merge - Testing CI Goldens Issue #36573

Closed

Updating cirrus fingerprint script to include goldens version #36690

Merged

Piinks reviewed Jul 23, 2019

View reviewed changes

Merge branch 'master' of github.com:flutter/flutter into cupertino_sw…

3ceef68

…itch

Piinks approved these changes Jul 24, 2019

View reviewed changes

justinmc approved these changes Jul 24, 2019

View reviewed changes

justinmc merged commit e08538c into flutter:master Jul 24, 2019

HansMuller mentioned this pull request Jul 25, 2019

On/off labels for CupertinoSwitch #36938

Closed

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Update visual style of CupertinoSwitch to match iOS 13 (flutter#36087)

a916f71

justinmc mentioned this pull request Jul 31, 2019

[iOS 13] Switch background sometimes incorrect #37312

Closed

HansMuller mentioned this pull request Oct 18, 2019

Upcoming Cupertino Library Features #43041

Closed

18 tasks

justinmc mentioned this pull request Jul 7, 2020

Implement new activity indicator for iOS14 #60179

Merged

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

Update visual style of CupertinoSwitch to match iOS 13 #36087

Update visual style of CupertinoSwitch to match iOS 13 #36087

Uh oh!

Conversation

brandondiamond commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Jul 12, 2019

Uh oh!

LongCatIsLooong commented Jul 16, 2019

Uh oh!

brandondiamond commented Jul 16, 2019

Uh oh!

brandondiamond commented Jul 16, 2019

Uh oh!

LongCatIsLooong commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

brandondiamond commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandondiamond commented Jul 17, 2019

Uh oh!

justinmc commented Jul 17, 2019

Uh oh!

brandondiamond commented Jul 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

brandondiamond commented Jul 18, 2019

Uh oh!

Uh oh!

brandondiamond commented Jul 18, 2019

Uh oh!

brandondiamond commented Jul 18, 2019

Uh oh!

LongCatIsLooong Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandondiamond commented Jul 19, 2019

Uh oh!

Uh oh!

Uh oh!

Piinks commented Jul 22, 2019

Uh oh!

brandondiamond commented Jul 22, 2019

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

brandondiamond commented Jul 23, 2019

Uh oh!

Piinks commented Jul 23, 2019

Uh oh!

brandondiamond commented Jul 23, 2019

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

brandondiamond commented Jul 12, 2019 •

edited

Loading

LongCatIsLooong commented Jul 16, 2019 •

edited

Loading

brandondiamond commented Jul 17, 2019 •

edited

Loading

codecov bot commented Jul 17, 2019 •

edited

Loading

LongCatIsLooong Jul 18, 2019 •

edited

Loading