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

[Material] Add support for hovered, pressed, and focused text color on Buttons. #33090

Merged
merged 36 commits into from
Jun 5, 2019

Conversation

johnsonmh
Copy link
Contributor

@johnsonmh johnsonmh commented May 21, 2019

Description

This PR adds support for defining the textColor of buttons when the button is in the hovered, focused, or pressed states.

This is useful for preserving the contrast ratio of the text on a button. Often, when buttons are interacted with, the contrast ratio can drop. This makes the text on buttons more difficult to read, and less accessible.

For example, we can make the blue text in this button darker on hover, and even darker on pressed.
giphy

Implementation

The implementation comes in three parts.

  1. Creating an enum for MaterialState, this includes, hovered, pressed, focused, dragged, disabled, and, error.
  2. Creating a MaterialStateColor class. MaterialStateColor has a resolve method that gets the color given a set of states. Or just use MaterialStateColor.resolveWith(...) and pass a callback that will be used to get a color given a set of states.
  3. Updating the RawMaterialButton to keep track of a Set<MaterialState>, the using it to get the color for the current state from MaterialStateColor (or just the color itself if the text color is not a MaterialStateColor).

Tests

I added the following tests:

  • Contrast ratio tests for FlatButton, OutlineButton, and RaisedButton in the hovered and focused states.
  • Tests that the text/icon of FlatButton, OutlineButton, and RaisedButton can be updated depending on the hovered, focused, and pressed states (using MaterialStateColor).
  • Tests for buttons in a blue color scheme. Normally in the blue color scheme, the button text is inaccessible in interactive states, but with MaterialStateColor, they can be fixed.
  • Tests that verify that passing a MaterialStatColor to a ButtonTheme still allows you to specify the hovered, pressed, and focused text color of a given button.
  • Tests that verify that passing a MaterialStateColor to textColor will cause disabledTextColor to be ignored. Instead, textColor in the disabled state will be used.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@johnsonmh johnsonmh requested a review from willlarche May 21, 2019 00:30
@shihaohong shihaohong added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 21, 2019
packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/colors.dart Outdated Show resolved Hide resolved
@zhancheng
Copy link

not only the button, but also the listTile needs the hover and press status to change the color

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

MaterialState and MaterialStateColor can stand alone; better to create material_state.dart.

There's a persistent typo that I eventually gave up on: if(foo) should be if (foo).

packages/flutter/lib/src/material/button.dart Show resolved Hide resolved
packages/flutter/lib/src/material/button.dart Show resolved Hide resolved
packages/flutter/lib/src/material/button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/colors.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/button_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/flat_button_test.dart Outdated Show resolved Hide resolved
Copy link
Member

@willlarche willlarche left a comment

Choose a reason for hiding this comment

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

SO good. I hope you're very proud.

packages/flutter/lib/src/material/material_state.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material_state.dart Outdated Show resolved Hide resolved
@googlebot

This comment has been minimized.

fix import

fix usage code
@johnsonmh johnsonmh force-pushed the feature-buttonStatefulColor branch from b49ed5b to 72e409b Compare June 3, 2019 15:06
@googlebot

This comment has been minimized.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 3, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This looks good, most of the feedback is just about only using expectLater where it's needed.

packages/flutter/lib/src/material/material_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material_state.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material_state.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/button_theme_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/outline_button_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/outline_button_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/raised_button_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/raised_button_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/raised_button_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/flutter/lib/src/material/material_state.dart Outdated Show resolved Hide resolved
@johnsonmh johnsonmh merged commit 1b87f55 into flutter:master Jun 5, 2019
@johnsonmh johnsonmh deleted the feature-buttonStatefulColor branch June 5, 2019 19:51
kiku-jw pushed a commit to kiku-jw/flutter that referenced this pull request Jun 14, 2019
…n Buttons. (flutter#33090)

* Support for stateful text colors in buttons

* Add color and a11y tests for buttons
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

None yet

8 participants