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: cuertino dialog action background blur effect #25076

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

akindone
Copy link
Contributor

@akindone akindone commented Dec 7, 2018

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Dec 7, 2018
@akindone
Copy link
Contributor Author

akindone commented Dec 8, 2018

Looking forward your review! @nataliesampsell @a14n
below images show the difference between before fix and after.
run on iPhone7 plus

flutter --version
Flutter 1.0.0 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 5391447fae (7 days ago) • 2018-11-29 19:41:26 -0800
Engine • revision 7375a0f414
Tools • Dart 2.1.0 (build 2.1.0-dev.9.4 f9ebf21297)

before-1
fixed-1
before-2
fixed-2

@matthew-carroll
Copy link
Contributor

Thanks for the PR to fix the issue.

At the moment I'm having trouble remembering why we used Rect.largest, but there was definitely a reason at the time, so I'm going to need a little time to test this PR and to discover that history.

I'm currently busy with some other things, so if you have some free cycles and you'd like to help verify this work even more, you might consider testing the dialog on top of more complex backgrounds.

One concern that I need to investigate is edge effects. We may have used Rect.largest to avoid rendering artifacts near the edges of the dialog. In your screenshots there do not appear to be any edge effects, but then again it's rendering on a very simple and solid background. The more unusual the content beneath the dialog, the better the chances that we discover edge effects.

@akindone
Copy link
Contributor Author

Thanks for the PR to fix the issue.

At the moment I'm having trouble remembering why we used Rect.largest, but there was definitely a reason at the time, so I'm going to need a little time to test this PR and to discover that history.

I'm currently busy with some other things, so if you have some free cycles and you'd like to help verify this work even more, you might consider testing the dialog on top of more complex backgrounds.

One concern that I need to investigate is edge effects. We may have used Rect.largest to avoid rendering artifacts near the edges of the dialog. In your screenshots there do not appear to be any edge effects, but then again it's rendering on a very simple and solid background. The more unusual the content beneath the dialog, the better the chances that we discover edge effects.

I have tried some other content beneath the dialog, like a video/another dialog/text and image, it looks good.

@xster
Copy link
Member

xster commented Dec 12, 2018

Thanks for the extra testing! LGTM

Matt can help you merge in when he's ready.

@xster
Copy link
Member

xster commented Jan 29, 2019

@akindone, thanks so much for taking the time to do the extra testing. It looks good to me. @matthew-carroll, I'm going to merge this in and watch out for it in the dev channel. Let us know if you have more concerns.

@xster xster merged commit 13a30c0 into flutter:master Jan 29, 2019
kangwang1988 pushed a commit to XianyuTech/flutter that referenced this pull request Feb 12, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants