Skip to content

Fix OutlineInputBorder with BorderRadius.zero is distorted #106849

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 1 commit into from
Sep 16, 2022
Merged

Fix OutlineInputBorder with BorderRadius.zero is distorted #106849

auto-submit merged 1 commit into from
Sep 16, 2022

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Jun 30, 2022 •
edited
Loading

Description

This PR fixes an OutlineInputBorder rendering bug. When OutlineInputBorder.borderRadius is BorderRadius.zero and the parent InputDecorator.labelText is set and shown on the border, the border is drawn using Canvas.drawPath and the path is distorted.

There were some miscalculations mainly related to using a temporary RRect with negative border radii.

This PR adds a private _clampRadius method to clamp negative border radii and builds the path without calling Path.addArc for corners with a zero border radius.
Edit: RRect radii clamping was added to the engine (see flutter/engine#36062 and flutter/engine#36106).

Before this PR (and before flutter/engine#36062 + flutter/engine#36106) :
before

Before this PR (and after flutter/engine#36062 + flutter/engine#36106) :
image

After :
after

Related Issue

Fixes #78855
Fixes #111086

Tests

Adds 1 test.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 30, 2022
@bleroux bleroux force-pushed the fix_outline_input_border_is_distorted branch from 75846ed to 952621b Compare June 30, 2022 05:55
@bleroux bleroux requested a review from justinmc June 30, 2022 06:55
@bleroux
Copy link
Contributor Author

bleroux commented Aug 3, 2022

Hey @justinmc!
Do you have any bandwidth to review this PR?
Or maybe pointed out someone who can review it?
Thanks!

bernaferrari
bernaferrari reviewed Sep 5, 2022
View reviewed changes
packages/flutter/lib/src/material/input_border.dart Outdated
Copy link
Contributor

@bernaferrari bernaferrari Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clampRadius could be a better name

@bleroux bleroux force-pushed the fix_outline_input_border_is_distorted branch 2 times, most recently from ec1e8ea to 589885f Compare September 8, 2022 06:23
@bleroux
Copy link
Contributor Author

bleroux commented Sep 8, 2022

@justinmc
Hi Justin! I’m Bruno from Nevercode :, working on issue fixing with @TahaTesser.
Kindly ping to ask for a review for this PR.
The triage team got a third report (#111086) related to the issue fixed by this PR. Previous reports were #78855 and #106545.
It would be so great to make progress on this. Thanks!

@bernaferrari
Copy link
Contributor

bernaferrari commented Sep 8, 2022 •
edited
Loading

If Justin is unavailable, you could try asking @gspencergoog for a review. He usually enjoys reviewing borders.

@bleroux bleroux requested a review from gspencergoog September 9, 2022 06:59
@gspencergoog gspencergoog mentioned this pull request Sep 9, 2022
Merged
gspencergoog
gspencergoog reviewed Sep 9, 2022
View reviewed changes
packages/flutter/lib/src/material/input_border.dart Outdated
Copy link
Contributor

@gspencergoog gspencergoog Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we do this in RRect.deflate/inflate (because deflate uses inflate with a negative value)? Or at least assert there when any of the radii go below zero?

There may be performance implications for actually clamping it there, but we shouldn't be asking for/allowing negative radii anyhow. RRect could assert, but also provide an inflate/deflate method that clamps radii (and suggest using that in the assert message).

(I've asked on Discord to see if there's a reason we don't already do this)

Copy link
Contributor

@gspencergoog gspencergoog Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a draft PR (flutter/engine#36062) to see if anything blows up when we try that.

Copy link
Contributor

@gspencergoog gspencergoog Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In talking to others about this, the consensus was that clamping is just a couple of instructions and we should just do the right thing. flutter/engine#36062 does clamping in deflate/inflate, and asserts if an RRect is created with a negative radius. As soon as the tests pass there, I'll move it to a non-draft PR, and then you can base this change on top of that (which should eliminate some of the code you needed to write).

Copy link
Contributor

@gspencergoog gspencergoog Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man. OK, so in order to assert that there aren't any negative radii given to RRect, I first need to have an easy way to clamp them, and add that into the Flutter repo code before I can merge the change that causes asserts when RRect gets a negative radius, so this is going to be a multi-step process to do the right thing. flutter/engine#36106

Copy link
Contributor Author

@bleroux bleroux Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I will update this PR as soon as flutter/engine#36062 and flutter/engine#36106 will land.

@bleroux bleroux force-pushed the fix_outline_input_border_is_distorted branch from 589885f to d7f09fb Compare September 15, 2022 09:20
@bleroux bleroux force-pushed the fix_outline_input_border_is_distorted branch from d7f09fb to 1776a0c Compare September 15, 2022 11:37
@gspencergoog
Copy link
Contributor

gspencergoog commented Sep 15, 2022

@bleroux OK, I've moved the RRect clamping through the process, so it's ready for you to update (if you haven't already). RRect should now clamp automatically to zero radius on deflate (or inflate with negative values), and will assert when it receives a negative radius. I'm not sure how much is left to do for this PR after accounting for that, but at the very least the testing is worthwhile.

@bleroux
Copy link
Contributor Author

bleroux commented Sep 16, 2022

I'm not sure how much is left to do for this PR after accounting for that, but at the very least the testing is worthwhile.

@gspencergoog That’s great to have RRect clamping at the engine level 👍
I had already updated the PR. Today I also updated the PR description with a new screenshot showing that there were remaining miscalculations to fix.

gspencergoog
gspencergoog approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Thanks for fixing this!

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 16, 2022
@auto-submit auto-submit bot merged commit 25e88cb into flutter:master Sep 16, 2022
@bleroux bleroux deleted the fix_outline_input_border_is_distorted branch September 16, 2022 19:14
@justinmc
Copy link
Contributor

justinmc commented Sep 16, 2022

@bleroux Sorry for missing this PR and thanks @gspencergoog for reviewing!

This was referenced Sep 17, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 17, 2022
@engine-flutter-autoroll
25e88cb Fix OutlineInputBorder with BorderRadius.zero is distorted (f…
b04a85c 
…lutter/flutter#106849)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 17, 2022
@engine-flutter-autoroll
25e88cb Fix OutlineInputBorder with BorderRadius.zero is distorted (f…
551911b 
…lutter/flutter#106849)
@gspencergoog gspencergoog mentioned this pull request Sep 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bernaferrari bernaferrari bernaferrari left review comments

@gspencergoog gspencergoog gspencergoog approved these changes

@justinmc justinmc Awaiting requested review from justinmc

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrong Border Panting In InputDecoration Border of a textfield is distorted upon focus.

4 participants

@bleroux @bernaferrari @gspencergoog @justinmc