Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Sep 22, 2021

Description

This fixes several problems with the delete button on Chip widgets:

  • Before this, the hit area for the delete button was always 1/3 of the chip size, leading to hit areas that are too wide on large chips, and too small on small chips. This modifies that so that on large chips, it takes up to 28 pixels to the left (in LTR locales, right on RTL) of the center of the delete button (basically a 48x48 square around the delete button, but clipped by the Chip), but is at a minimum up to the left edge of the delete button of the chip on small chips.
  • The ripple on the delete button was not being fired in all situations, and when it was, it was clipped to the 16x16 delete icon, making it almost invisible. Now it expands towards the center of the delete button, and stops just short of the edge of the chip.
  • The delete button didn't have a focus node, so couldn't be accessed via the keyboard focus traversal. Now it has a focus node again.
  • Fixes a small iteration issue in the mock canvas that was throwing an exception instead of showing where the draw operations differed.

This change does not revert to having two ripples for a delete press: there is still only one, since the hit testing separates the delete button InkWell from the main chip (i.e. it doesn't regress b/140930220).

Before (clicking on each active area of the chip):

before_chip.mp4

After (clicking on each active area of the chip):

after_chip.mp4

Related Issues

Tests

  • Added a test for the delete button taking up the right amount of the chip.
  • Updated tests to allow for more than one InkWell, but still check for a single ripple.
  • Added a test to make sure the delete button is focusable.
  • Added a test to make sure that chips without onPressed set, but with onDelete set still ripple.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 22, 2021
@google-cla google-cla bot added the cla: yes label Sep 22, 2021
@gspencergoog gspencergoog marked this pull request as ready for review September 22, 2021 18:14
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of small comments. Nice cleanup.

@fluttergithubbot fluttergithubbot merged commit cf89a78 into flutter:master Sep 24, 2021
gspencergoog added a commit that referenced this pull request Sep 28, 2021
…p. (#90845)

This adjusts the size of the delete button so that it takes up just slightly less than half of the chip, so that legacy tests that tap on the center of the chip still hit the chip, and not the delete button.

A follow-on change for #90531
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
…p. (flutter#90845)

This adjusts the size of the delete button so that it takes up just slightly less than half of the chip, so that legacy tests that tap on the center of the chip still hit the chip, and not the delete button.

A follow-on change for flutter#90531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

3 participants