Skip to content

Make UnderlineInputBorder consistent #124153

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 4 commits into from
Nov 15, 2023
Merged

Make UnderlineInputBorder consistent #124153

auto-submit merged 4 commits into from
Nov 15, 2023

Conversation

@bernaferrari
Copy link
Contributor

@bernaferrari bernaferrari commented Apr 4, 2023 •
edited
Loading

This was easy to implement. I like the result, I think borderRadius.zero -> borderRadius.circular makes a nice transition, and many places (like macOS) use an effect similar to this PR, while Google doesn't use anywhere (yet). I'm curious if it is going to break goldens or google testing.

ttt

What do you think? cc @HansMuller @gspencergoog. Is this something you want, should I ask the community, or do you prefer the current one?

Side effects:

  • This makes strokeAlign work with UnderlineInputBorder (TODO: fix drawLine when borderRadius is zero).
  • This is faster than the current implementation (clip is slow on Skia). 🚀
  • We could just call BoxBorder._paintNonUniformBorder (if it weren't private). Single LOC implementation.
  • Web does this by default:
    image
  • Apparently no tests fail and most usages around (via code search) seem to be without a borderRadius.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 4, 2023
@bernaferrari bernaferrari force-pushed the UnderlineInputBorder branch 3 times, most recently from 86a0f29 to 3fe5717 Compare April 5, 2023 19:09
@goderbauer goderbauer requested a review from justinmc May 3, 2023 12:32
justinmc
justinmc reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

@bernaferrari Weren't you and I discussing this on another issue somewhere? Could you link that if you still have the link? And post the code to produce these fields.

Also, can you clarify what the difference is between "Currently" and "Proposal" in your image? Should the top one say BorderRadius.zero?

I couldn't get the Google tests to show up here but I will run them locally.

packages/flutter/lib/src/material/input_border.dart Outdated
Copy link
Contributor

@justinmc justinmc May 8, 2023

Choose a reason for hiding this comment

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

Why not use clampDouble on these?

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023

Choose a reason for hiding this comment

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

Because there are two values, so the other possible way would be

Radius(clampDouble(min: 0, rect.tlRadiusX + insets.left), clampDouble(min: 0, rect.tlRadiusY + inset.top))

Do you prefer that way? I can make it.

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023

Choose a reason for hiding this comment

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

Oh, good news: I copied the code from what I'm doing on BoxDecoration. When my other PR gets merged, this will be redundant and I'll be able to use the BoxDecoration.paintNonUniformBorder.

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Which one do you prefer? If you prefer the top one, I can later modify in the BoxDecoration:

image

Copy link
Contributor

@justinmc justinmc May 9, 2023

Choose a reason for hiding this comment

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

I guess the clampDouble way doesn't look much worse, I say use that. The linter rule is there because .clamp is much slower, see #103917.

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023

Choose a reason for hiding this comment

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

But clamp from Radius uses clampDouble
image

@justinmc
Copy link
Contributor

justinmc commented May 8, 2023

I don't see any Google test failures.

@bernaferrari bernaferrari reopened this May 9, 2023
@bernaferrari
Copy link
Contributor Author

bernaferrari commented May 9, 2023 •
edited
Loading

Ops, I was writing the comment and pressed the wrong button.

I don't think so, I commented in the Framework channel from Discord. Hixie preferred this, a few others preferred the old one. There was no consensus.
https://discord.com/channels/608014603317936148/608021234516754444/1092856395809955910

The difference is that the old one clips, so for example, if you have a super large radius, the line would be really short, it only shows the "rectangular" part. The proposal is to follow the curve, embrace the radius. It is faster, better, more consistent with other platforms and non breaking. But kind of big. The screenshot shows the code diff, from path to drrect:

Screenshot_20230509_102027_Chrome

(if you accept, please don't autosubmit, I have another PR on the way that will allow to simplify this further, but the result will be the same, just less LOC).

justinmc
justinmc approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for the Discord link, that's what I was thinking of. I'm on board with "Proposal" approach.

Leaving this with no autosubmit for now as requested.

packages/flutter/lib/src/material/input_border.dart Outdated
Copy link
Contributor

@justinmc justinmc May 9, 2023

Choose a reason for hiding this comment

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

Nit: Could this be static?

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023

Choose a reason for hiding this comment

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

Yes (but why?).

But this is going to probably be replaced by BoxBorder.paintNonUniformPath. I just need to get that approved. Shouldn't take more than a few days.

Copy link
Contributor Author

@bernaferrari bernaferrari May 9, 2023

Choose a reason for hiding this comment

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

It is funny I can make the BoxBorder changes in this commit or in the other one, lol.. But the other one seems to be in "almost approval" phase, so let's see.

@bernaferrari bernaferrari force-pushed the UnderlineInputBorder branch from 3fe5717 to 0ed7dc2 Compare May 9, 2023 18:44
@Hixie
Copy link
Contributor

Hixie commented Jul 18, 2023

Spoke with @bernaferrari, he says this is blocked on #124417

@bernaferrari
Copy link
Contributor Author

bernaferrari commented Jul 18, 2023

Yes, thanks! I forgot to write this here lol

@bernaferrari bernaferrari force-pushed the UnderlineInputBorder branch 4 times, most recently from 97533b2 to cc93222 Compare August 17, 2023 20:52
bernaferrari
bernaferrari commented Aug 17, 2023
View reviewed changes
packages/flutter/lib/src/material/input_border.dart Outdated
Comment on lines +250 to +251
Copy link
Contributor Author

@bernaferrari bernaferrari Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@justinmc Ok, heeeere we go! Not sure I need another review (probably not), just double checking. I implemented what I wanted to, and the only slightly "odd" thing is this.
Reason: If topLeft/topRight borders are there, the paint causes weird anti-alias artifacts to show, so making them square, and limiting the bottomLeft/Right to h/2 prevents the issue from ever appearing.
Consequences: theoretically an elliptical border(999, 0) wouldn't be possible anymore (clamp to h/2), but a) it is not breaking, b) it is already rare in standard widgets, c) this would be even more rare. I think it is ok.
Screenshot of what I'm trying to avoid:
image

Hixie
Hixie approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@Hixie Hixie left a comment

Choose a reason for hiding this comment

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

LGTM

@Hixie
Copy link
Contributor

Hixie commented Nov 14, 2023

@bernaferrari Ok let's try landing it!

@bernaferrari bernaferrari force-pushed the UnderlineInputBorder branch 3 times, most recently from d6c9e19 to b026914 Compare November 15, 2023 16:46
@bernaferrari bernaferrari marked this pull request as ready for review November 15, 2023 16:57
@bernaferrari bernaferrari added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2023
@auto-submit auto-submit bot merged commit 2afec77 into flutter:master Nov 15, 2023
@bernaferrari bernaferrari deleted the UnderlineInputBorder branch November 15, 2023 23:14
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 16, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 16, 2023
@engine-flutter-autoroll
2afec77 Make UnderlineInputBorder consistent (flutter/flutter#124153)
f2763a5
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 16, 2023
@engine-flutter-autoroll
Roll Flutter from e8c2bb1 to 53a57ad (39 revisions) (#5412)
52a5342 
Roll Flutter from e8c2bb1 to 53a57ad (39 revisions)

flutter/flutter@e8c2bb1...53a57ad

2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8ab189b77b8d to 2e9f0df868b3 (1 revision) (flutter/flutter#138543)
2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 622fa0614412 to 8ab189b77b8d (1 revision) (flutter/flutter#138533)
2023-11-16 leroux_bruno@yahoo.fr [flutter_tools] - Add `queries` section to Android manifest file (flutter/flutter#137207)
2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8aff9c134b8f to 622fa0614412 (1 revision) (flutter/flutter#138532)
2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3cfcdebe8623 to 8aff9c134b8f (18 revisions) (flutter/flutter#138529)
2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 30327eae0802 to 3cfcdebe8623 (1 revision) (flutter/flutter#138515)
2023-11-15 42216813+eliasyishak@users.noreply.github.com Catch error for missing directory in `FontConfigManager` (flutter/flutter#138496)
2023-11-15 bernaferrari2@gmail.com Make `UnderlineInputBorder` consistent (flutter/flutter#124153)
2023-11-15 gspencergoog@users.noreply.github.com Prepare `ShortcutActivator` and `ShortcutManager` to migrate to `KeyEvent` from `RawKeyEvent`. (flutter/flutter#136854)
2023-11-15 58529443+srujzs@users.noreply.github.com Pin package:web 0.4.0 (flutter/flutter#138428)
2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland VelocityTracker update (#132291)" (flutter/flutter#138512)
2023-11-15 yjbanov@google.com [web] skip flaky overflow_clipbehavior_none.cupertino.0.png golden check (flutter/flutter#138498)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7c2b8d520b3d to 30327eae0802 (2 revisions) (flutter/flutter#138502)
2023-11-15 katelovett@google.com Reland VelocityTracker update (#132291) (flutter/flutter#137381)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from f58dac64ad1a to 7c2b8d520b3d (1 revision) (flutter/flutter#138499)
2023-11-15 katelovett@google.com Fix 2D tap to stop scrolling (flutter/flutter#138442)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from d22d063ac9f6 to f58dac64ad1a (2 revisions) (flutter/flutter#138494)
2023-11-15 41873024+droidbg@users.noreply.github.com SemanticOwner should dispatch creation and disposal events (flutter/flutter#138388)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from ecaf9442034d to d22d063ac9f6 (5 revisions) (flutter/flutter#138489)
2023-11-15 engine-flutter-autoroll@skia.org Roll Packages from 428ba3e to 0cd2378 (1 revision) (flutter/flutter#138482)
2023-11-15 fluttergithubbot@gmail.com Marks Mac_android hot_mode_dev_cycle__benchmark to be unflaky (flutter/flutter#138472)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from a7a48a68e6f8 to ecaf9442034d (1 revision) (flutter/flutter#138468)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 976edd5192d1 to a7a48a68e6f8 (3 revisions) (flutter/flutter#138463)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from a7f2267dd1f4 to 976edd5192d1 (1 revision) (flutter/flutter#138462)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc5bbd3b9ebe to a7f2267dd1f4 (1 revision) (flutter/flutter#138459)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from d7ca057b891f to bc5bbd3b9ebe (2 revisions) (flutter/flutter#138457)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1347413470b7 to d7ca057b891f (1 revision) (flutter/flutter#138456)
2023-11-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from c5a067b637f4 to 1347413470b7 (5 revisions) (flutter/flutter#138452)
2023-11-15 xubaolin@oppo.com Reland [SingleChildScrollView] Correct the offset pixels if it is out of range during layout (flutter/flutter#136871)
2023-11-14 katelovett@google.com Add to TableCell docs (flutter/flutter#138258)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from f15b259fe98c to c5a067b637f4 (4 revisions) (flutter/flutter#138441)
2023-11-14 49699333+dependabot[bot]@users.noreply.github.com Bump dessant/lock-threads from 4.0.1 to 5.0.0 (flutter/flutter#138437)
2023-11-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.5 to 2.22.6 (flutter/flutter#138438)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from eba757803a6f to f15b259fe98c (1 revision) (flutter/flutter#138429)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 603bdd48df8a to eba757803a6f (3 revisions) (flutter/flutter#138425)
2023-11-14 42216813+eliasyishak@users.noreply.github.com Unified analytics migration for `CodeSizeAnalysis` (flutter/flutter#138351)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 777dcb99f6e0 to 603bdd48df8a (1 revision) (flutter/flutter#138421)
2023-11-14 goderbauer@google.com Run all tests in examples/ (flutter/flutter#138374)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b3fd80812c3 to 777dcb99f6e0 (2 revisions) (flutter/flutter#138420)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,ychris@google.com on the revert to ensure that a human
is aware of the problem.

...
@HansMuller HansMuller mentioned this pull request Nov 17, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
2afec77 Make UnderlineInputBorder consistent (flutter/flutter#124153)
800926d
@ltOgt ltOgt mentioned this pull request Feb 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

@Hixie Hixie Hixie approved these changes

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.

3 participants

@bernaferrari @justinmc @Hixie