Fix filled color is wrong for a focused and hovered TextField #146976

bleroux · 2024-04-18T09:07:23Z

Description

This PR fixes the filled color for a focused and hovered text field.
Before this PR, the filled color for a focused text field did not change when hovered, after this PR the filled color is blended with the hover color.

The change removes a isFocused check which deactivated the blending. This check was introduced in #32776, at that time it was needed because there was also a focus color animation. Sometimes later, the focus animation was removed, see #33062, but the flag was not removed.

Before:

Enregistrement.de.l.ecran.2024-04-18.a.08.45.01.mov

After:

Enregistrement.de.l.ecran.2024-04-18.a.08.46.54.mov

Related Issue

Fixes #146573

Tests

Adds 1 tests.

Renzo-Olivares · 2024-04-19T22:45:04Z

Hi @bleroux, thanks for the contribution and taking the time to investigate where the isFocused flag came. This change LGTM, but is there some spec that this is aligning to? Nothing stands out from a quick glance at the m3 TextField spec, but I do see this spec regarding states in general https://m3.material.io/foundations/interaction/states/state-layers and how focused/hovered and other states can be combined.

bleroux · 2024-04-22T12:28:20Z

Hey @Renzo-Olivares,
As you pointed the main source of information is: https://m3.material.io/foundations/interaction/states/state-layers.
The M3 text field spec references the state layer in the table found in this section: https://m3.material.io/components/text-fields/specs#f967d3f6-0139-43f7-8336-510022684fd1

Note that 'State layer' is only mentionned for the hovered state, other states leads to a change of the container fill color.

In this PR, I fixed the main issue (not seeing hover state for a focus filled TextField).
I plan to file a follow up PR that will try to make the hover color compliant with the spec. For the moment, when hovered, the filled color is blendend with theme default color (for instance, Colors.black.withOpacity(0.04), for a light theme) which is not the one specified in the M3 spec (which is onSurface color with 0.08 opacity as defined in the screenshot above).
I will also have a look at how the hover state is rendered when the field is not focused because the current implementation might not be compliant.

Renzo-Olivares · 2024-04-22T19:03:49Z

@bleroux thank you for the explanation! That sounds good to me. There are a few failing image tests but they all look expected given the changes in this PR.

Renzo-Olivares

LGTM

bleroux · 2024-04-23T08:13:02Z

@Renzo-Olivares
Thanks for the review!

There are a few failing image tests but they all look expected given the changes in this PR.
Can you approve those changes? I don't have access to approve them.

Renzo-Olivares · 2024-04-23T22:35:15Z

@bleroux when you get a chance can you rebase this PR to kick off google testing again?

bleroux · 2024-04-24T19:03:26Z

@bleroux when you get a chance can you rebase this PR to kick off google testing again?

@Renzo-Olivares I rebased once some hours ago and Google testing step was still failing.
I rebased another time just now, let see how it goes.

…flutter/flutter#146976)

flutter/flutter@dba4f77...5d3bca4 2024-04-25 [email protected] Roll Flutter Engine from 6b33890a4701 to 674890ce7141 (1 revision) (flutter/flutter#147365) 2024-04-25 [email protected] [native_assets] Use kernel concatenation (flutter/flutter#147158) 2024-04-25 [email protected] Roll Flutter Engine from ccd57f5a4bc4 to 6b33890a4701 (2 revisions) (flutter/flutter#147359) 2024-04-25 [email protected] Roll Flutter Engine from 9de649ed2926 to ccd57f5a4bc4 (1 revision) (flutter/flutter#147356) 2024-04-25 [email protected] Roll Flutter Engine from 2f48456a425a to 9de649ed2926 (2 revisions) (flutter/flutter#147354) 2024-04-25 [email protected] Roll Flutter Engine from f48f3b6a0172 to 2f48456a425a (1 revision) (flutter/flutter#147351) 2024-04-25 [email protected] Roll Flutter Engine from b30c0a765680 to f48f3b6a0172 (8 revisions) (flutter/flutter#147348) 2024-04-25 [email protected] Allow the SceneBuilder, PictureRecord, and Canvas constructor calls from the rendering layer to be hooked (flutter/flutter#147271) 2024-04-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.3 to 4.1.4 (flutter/flutter#147334) 2024-04-24 [email protected] Fix filled color is wrong for a focused and hovered TextField (flutter/flutter#146976) 2024-04-24 [email protected] Roll Flutter Engine from b5d5832f7144 to b30c0a765680 (14 revisions) (flutter/flutter#147336) 2024-04-24 [email protected] Run new_gallery__transition_perf on mokey in staging (flutter/flutter#147339) 2024-04-24 [email protected] Remove hidden dependencies on the default goldenFileComparator. (flutter/flutter#146956) 2024-04-24 [email protected] Add create app and plugin templates for Swift Package Manager (flutter/flutter#147082) 2024-04-24 [email protected] Add support for overriding `reverseCurve` with `ExpansionTile.expansionAnimationStyle` (flutter/flutter#147103) 2024-04-24 [email protected] Mark firebase tests as `bringup: true` (flutter/flutter#147338) 2024-04-24 [email protected] Add Valentin Vignal to AUTHORS (flutter/flutter#147314) 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 [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…flutter/flutter#146976)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 18, 2024

bleroux requested a review from justinmc April 18, 2024 09:12

bleroux force-pushed the fix_filled_color_is_wrong_for_focused_and_hovered_text_field branch from e8d260f to 81abb72 Compare April 19, 2024 12:22

Renzo-Olivares approved these changes Apr 22, 2024

View reviewed changes

bleroux force-pushed the fix_filled_color_is_wrong_for_focused_and_hovered_text_field branch from 81abb72 to 59e310e Compare April 24, 2024 06:23

bleroux added 2 commits April 24, 2024 21:01

Fix filled color is wrong for a focused and hovered TextField

b706890

Add missing commas

11c2c17

bleroux force-pushed the fix_filled_color_is_wrong_for_focused_and_hovered_text_field branch from 59e310e to 11c2c17 Compare April 24, 2024 19:01

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 24, 2024

auto-submit bot merged commit 99411e5 into flutter:master Apr 24, 2024

bleroux deleted the fix_filled_color_is_wrong_for_focused_and_hovered_text_field branch April 25, 2024 06:35

engine-flutter-autoroll mentioned this pull request Apr 25, 2024

Roll Flutter from dba4f7747441 to 5d3bca4500b1 (17 revisions) flutter/packages#6612

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 25, 2024

99411e5 Fix filled color is wrong for a focused and hovered TextField (…

fb60180

…flutter/flutter#146976)

HansMuller mentioned this pull request Apr 26, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

99411e5 Fix filled color is wrong for a focused and hovered TextField (…

06cf313

…flutter/flutter#146976)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix filled color is wrong for a focused and hovered TextField #146976

Fix filled color is wrong for a focused and hovered TextField #146976

Uh oh!

bleroux commented Apr 18, 2024 •

edited

Loading

Uh oh!

Renzo-Olivares commented Apr 19, 2024

Uh oh!

bleroux commented Apr 22, 2024 •

edited

Loading

Uh oh!

Renzo-Olivares commented Apr 22, 2024

Uh oh!

Renzo-Olivares left a comment

Uh oh!

bleroux commented Apr 23, 2024

Uh oh!

Renzo-Olivares commented Apr 23, 2024

Uh oh!

bleroux commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix filled color is wrong for a focused and hovered TextField #146976

Fix filled color is wrong for a focused and hovered TextField #146976

Uh oh!

Conversation

bleroux commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Tests

Uh oh!

Renzo-Olivares commented Apr 19, 2024

Uh oh!

bleroux commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renzo-Olivares commented Apr 22, 2024

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux commented Apr 23, 2024

Uh oh!

Renzo-Olivares commented Apr 23, 2024

Uh oh!

bleroux commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bleroux commented Apr 18, 2024 •

edited

Loading

bleroux commented Apr 22, 2024 •

edited

Loading