Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Nov 20, 2024

Description

This PR fixes the DatePicker overlay colors for the selected days.
Before this PR, overlays were obscured by the selected day backgound.
This fix simply replaces a DecoratedBox with an Ink to make the overlays visible.
Combined with #159072 which fixes InkWell overlay color resolution related to the selected state, this PR fixes Date picker overlay colors aren't applied on MaterialState.selected state.

Before, no overlay visible for the selected day when hovered, focused, or pressed:

Enregistrement.de.l.ecran.2024-11-20.a.16.10.38.mov

After, overlay is visible for the selected day when hovered, focused, or pressed (color change is slight as defined with M3 defaults):

Enregistrement.de.l.ecran.2024-11-20.a.16.09.14.mov

Related Issue

Fixes Date picker overlay colors aren't applied on MaterialState.selected state.

Tests

Adds 12 tests.
Updates several existing tests (those tests were looking for a DecoratedBox, make them look for an Ink).

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 20, 2024
@bleroux bleroux force-pushed the fix_date_picker_day_highlights_hidden_by_decoration branch from 1f12bf0 to 461ca17 Compare November 20, 2024 17:11
@bleroux bleroux force-pushed the fix_date_picker_day_highlights_hidden_by_decoration branch from 461ca17 to 872dfda Compare November 20, 2024 17:49
@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Nov 20, 2024
@bleroux bleroux requested a review from justinmc November 20, 2024 19:04
Comment on lines +778 to +782
MaterialInkController findDayGridMaterial(WidgetTester tester) {
// All days are painted on the same Material widget.
// Use an arbitrary day to find this Material.
return Material.of(tester.element(find.text('17')));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

😍😍

This is wonderful, thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
It took me several hours to figure out that there is not one Material for each day.
This was mainly because existing tests were carefully using the day they are interested in to look up the Material. It was ok, until I spent hours fixing one test where the circle painted for another day collides with the one I was interested in.
I created this utility function to save time for future contributors who will work on the date picker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have some mixed feelings about the Ink widget—it seems like it'd help performance by consolidating a bunch of widgets into a single ink controller, but it also has the tendency to introduce jank, and while it appears to work very similar to DecoratedBox, some things are super confusing for anyone who doesn't have a solid grasp of how painting on a Material works.

@bleroux bleroux requested a review from TahaTesser November 25, 2024 08:44
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM!

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 25, 2024
@auto-submit auto-submit bot added this pull request to the merge queue Nov 25, 2024
Merged via the queue into flutter:master with commit 8509d78 Nov 25, 2024
77 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 25, 2024
@bleroux bleroux deleted the fix_date_picker_day_highlights_hidden_by_decoration branch November 25, 2024 19:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024
@bleroux
Copy link
Contributor Author

bleroux commented Nov 28, 2024

Reason for revert: This PR depends on #159072 which was flagged as a perf regression in #159337. Reverting to see if the perf regression was real or not. See #159337 (comment) for context.

@bleroux bleroux added the revert Autorevert PR (with "Reason for revert:" comment) label Nov 28, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 28, 2024

Time to revert pull request flutter/flutter/159203 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Nov 28, 2024
bleroux added a commit to NevercodeHQ/flutter that referenced this pull request Nov 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2024
#159583)

Reverts #159203 because it
depends on #159072 which was
flagged as a perf regression in
#159337.

Reverting both PRs to see if the perf regression was really related to
this change or was impacted by another change. See
#159337 (comment)
for context.
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024
#159839)

Reland #159203 without change.
The initial PR was reverted in
#159583.

Fixes [Date picker overlay colors aren't applied on
MaterialState.selected
state](#130586).
@reidbaker reidbaker mentioned this pull request Dec 13, 2024
11 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Date picker overlay colors aren't applied on MaterialState.selected state.

3 participants