fix(material/core): add checkmark for single-select #25962

zarend · 2022-11-11T22:08:07Z

Add checkmark to mat-option for single-select. Fix a11y issues where selected state is visually communicated with color alone. Communicate selection with both color and a checkmark indicator. Affect components that use mat-option for single-selection, which include select and autocomplete.

Add an appearance Input to mat-pseudo-checkbox. "full" appearance renders a checkbox, which is the current behavior. Render "full" appearance by default. "minimal" apperance renders only a checkmark.

Summary of API and behavior changes:

Add an @Input appearance to pseudo-checkbox with options for "full" and "minimal".
mat-option renders "minimal" appearance for single-select, which looks like a checkmark.
Select and autocomplete components display checkmark on selected option.

What's not changing:

does not affect multiple selection
mat-option renders "full" apperance by default, which is same as existing behavior

Fixes: #25961

src/material/core/option/option.html

crisbeto · 2022-11-14T14:12:48Z

src/material/core/option/option.html

@@ -1,5 +1,6 @@
-<mat-pseudo-checkbox *ngIf="multiple" class="mat-mdc-option-pseudo-checkbox"


Looking at the screenshot of mat-autocomplete, a couple of things stand out:

The checkmark isn't centered inside the option.

The checkmark color should match the text color.

I don't know why it's not centering correctly for autocomplete, but I'll look into that.

For 2, that's because the checkmark uses accent color by default. I think the accent color makes more sense for "full" appearance than "minimal" appearance. Are you suggesting that we shouldn't use the theming colors for checkmark, and always try to match the text color? Or pointing out that it doesn't look right?

// Default to the accent color. Note that the pseudo checkboxes are meant to inherit the // theme from their parent, rather than implementing their own theming, which is why we // don't attach to the `mat-*` classes. Also note that this needs to be below `.mat-primary` // in order to allow for the color to be overwritten if the checkbox is inside a parent that // has `mat-accent` and is placed inside another parent that has `mat-primary`.

I suspect that it defaults to accent, because mat-checkbox does too and it was written to match mat-checkbox as closely as possible. We could leave on the default and have the consuming component override the color.

That being said, I found the current color a little weird since your select screenshot looks right, but the autocomplete does not.

I changed this to use a mathematic formula to place the checkmark. Hopefully this fixes the centering. Does it look centered to you? I can barely tell the difference between before and after.

For 2, I sent out #25983 to fix autocomplete not applying the right theme to the panel.

does this PR include the changes from the PR you linked? I still see the autocomplete check showing up as the wrong color

#25983 landed, and I believe the appearance for the autocomplete has been corrected. Could someone please confirm that this is the correct appearance? Is the font color supposed to change when there's a checkmark? See attached screenshots of both with and without selection indicators.

zarend · 2022-11-17T17:33:40Z

rebased with upstream/main. Maybe this will fix the issue with dev-app deployment 🤞

github-actions · 2022-11-17T17:40:41Z

Deployed dev-app to: https://ng-dev-previews-comp--pr-angular-components-25962-0077-q1n3hdgg.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

mmalerba

LGTM aside from the color issue

src/material/core/selection/pseudo-checkbox/_pseudo-checkbox-common.scss

crisbeto

#25962 (comment) should be resolved before we move forward.

zarend · 2022-11-18T16:59:29Z

The autocomplete theming fix has landed. I rebased this branch, and we can see that the colors are corrected.

zarend · 2023-01-05T17:15:28Z

@mmalerba @crisbeto could you review this again please as it has changed. Since last reviewed, added opt-outs for Select and Autocomplete.

I have a few questions:

Do we need an opt-out for when used outside of select or autocomplete? As implemented, the opt-out only affect mat-option inside of a Select or Autocomplete. For ripple, there's a way to disable it from the Select/Autocomplete and from the mat-option too. Should hideSingleSelectionIndicator do the same?
Could you take a look at _syncParentProperties? I ran into an issue where autocomplete wouldn't work when testing on the autocomplete overview demo (/autocomplete). Clicking the "Hide Single-Selection Indicators" didn't seem to do anything. It seemed like change detection wasn't running because we mutated the _parent property on MatOption and it uses on-push change detection. I couldn't repro on Select, but I did the same fix for Select anyways.
For MatAutocomplete, would it be better to this another way, like assigning defaults to the base class, or using inject function? That way we don't have the change the constructor of the derived class?

src/material/autocomplete/autocomplete.ts

crisbeto · 2023-01-05T18:10:49Z

src/material/autocomplete/autocomplete.ts

+  }
+  set hideSingleSelectionIndicator(value: BooleanInput) {
+    this._hideSingleSelectionIndicator = coerceBooleanProperty(value);
+    this._syncParentProperties();


In what case does this become a problem? E.g. is it only if the binding changes while the autocomplete is open or does it not get picked up on open either?

It doesn't get picked up on open. Although, it does get picked up when selecting an option, selecting the same option again, or selecting a different option. It seems to me that selecting an option triggers chance detection by conincidence, which cases the child component to detect the change to hideSingleSelectionIndicator.

// option.ts /** Whether to display checkmark for single-selection. */ get hideSingleSelectionIndicator(): boolean { return !!(this._parent && this._parent.hideSingleSelectionIndicator); }

Both MatOption and MatAutocomplete use on-push change detection. I believe that means that mutating _parent will not trigger change detection on MatOption. That's why we have to manually call markForCheck. Am I understanding the change detection correctly :)?

Here's a video capture of the behavior I see when I remove _syncParentProperties.

without._syncParentProperties.720p.mov

I see. I'm surprised that this didn't come up for disableRipple or multiple who follow the same pattern.

I can reproduce this problem with disableRipple. I think this has been happening all along, but there haven't been any issues filed for it because disableRipple and multiple are changed very rarely in practice.

I added a disableRipple option to the autocomplete demo and here is a video of the reproduction. It's kinda hard to see in the video, so here are the steps that happen.

(Ripple starts off enabled.

Click "Alabama"

(displays ripple animation)

Click "Alabama" again

(displays ripple animation)

Click "TOGGLE DISABLE RIPPLE"

Click "Alabama"

(displays ripple animation. Is not supposed to display ripple animation)

Click "Alabama" again

(does not display ripple animation)

autocomplete-disableRipple.mov

crisbeto · 2023-01-05T18:40:36Z

src/material/autocomplete/autocomplete.ts

+  }
+  set hideSingleSelectionIndicator(value: BooleanInput) {
+    this._hideSingleSelectionIndicator = coerceBooleanProperty(value);
+    this._syncParentProperties();


I see. I'm surprised that this didn't come up for disableRipple or multiple who follow the same pattern.

src/material/autocomplete/autocomplete.ts

mmalerba

LGTM

zarend · 2023-01-11T23:46:48Z

Since this was last reviewed, changed out it puts the checkmark on the right of the mat option. Using margin to make space between the checkmark and the list item instead of using flex-grow: 1;. Increasing the size of the list item content causes unintended layout changes.

The only file I touched was option.scss, in case anyone wants to see what changed.

Add checkmark to mat-option for single-select. Fix a11y issues where selected state is visually communicated with color alone. Communicate selection with both color and a checkmark indicator. Affect components that use mat-option for single-selection, which include select and autocomplete. Add an `appearance` Input to mat-pseudo-checkbox. "full" appearance renders a checkbox, which is the current behavior. Render "full" appearance by default. "minimal" apperance renders only a checkmark. Add an opt-out to Selection and Autocomplete components for checkmark indicators for single-selection. Add both Input and DI token to specify if checkmark indicators are hidden for single-select. By default display checkmark indicators for single-selection. If both DI token and Input are specified, the Input wins. Does not affect multiple-selection. Does not affect legacy components. Summary of API and behavior changes: - Add an `@Input appearance` to pseudo-checkbox with options for "full" and "minimal". "full" appearance is same and current appearance, which renders a checkmark inside a box. "minimal" appearance renders the checkmark without a box. - By default, mat-option renders "minimal" appearance for single-select. - Add `hideSingleSelectionIndicator` property to `MatOptionParentComponent`. mat-option hides single-selection indicator when specified by its parent. - by default, Select and Autocomplete components display checkmark on selected option. - Both Autocomplete and Select add `@Input hideSingleSelectionIndicator` to specify if checkmark indicator is displayed for single-selection. - Add `hideSingleSelectionIndicator` property to `MatSelectConfig`, which specifies default value for `hideSingleSelectionIndicator`. - Add `hideSingleSelectionIndicator` property to `MatAutocompleteDefaultOptions`, which specifies default value for `hideSingleSelectionIndicator`. Fixes: angular#25961

…ckmark indicators Update the accessibility section on checkmark indicators for single-selection. Add instructions to always communicate selection with icon indicators. Fulfill documentation needs as follow-up for angular#25962.

For Select and Autocomplete components, update the accessibility section on heckmark indicators for single-selection. Add instructions to always communicate with icon indicators. Fulfill documentation needs as follow-up for PR angular#25962.

For Select and Autocomplete components, update the accessibility section on heckmark indicators for single-selection. Add instructions to always communicate with icon indicators. Fulfill documentation needs as follow-up for PR #25962.

For Select and Autocomplete components, update the accessibility section on heckmark indicators for single-selection. Add instructions to always communicate with icon indicators. Fulfill documentation needs as follow-up for PR #25962. (cherry picked from commit 6593954)

angular-automatic-lock-bot · 2023-02-18T00:04:14Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

zarend added P2 The issue is important to a large percentage of users, with a workaround Accessibility This issue is related to accessibility (a11y) area: material/autocomplete area: material/core area: material/select labels Nov 11, 2022

zarend requested a review from mmalerba November 11, 2022 22:08

zarend requested review from crisbeto, jelbourn, devversion and andrewseguin as code owners November 11, 2022 22:08

zarend force-pushed the minimal-pseudo-checkbox branch 4 times, most recently from d3b277b to a64288e Compare November 12, 2022 01:06

crisbeto reviewed Nov 14, 2022

View reviewed changes

zarend force-pushed the minimal-pseudo-checkbox branch from a64288e to 38e8754 Compare November 15, 2022 03:00

crisbeto added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Nov 15, 2022

zarend mentioned this pull request Nov 15, 2022

fix(material/core): display checkmark for single-selection #25934

Closed

mmalerba added dev-app preview When applied, previews of the dev-app are deployed to Firebase and removed dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Nov 15, 2022

zarend force-pushed the minimal-pseudo-checkbox branch from 38e8754 to d41d563 Compare November 17, 2022 17:33

mmalerba reviewed Nov 18, 2022

View reviewed changes

src/material/core/selection/pseudo-checkbox/_pseudo-checkbox-common.scss Show resolved Hide resolved

mmalerba added the target: minor This PR is targeted for the next minor release label Nov 18, 2022

crisbeto requested changes Nov 18, 2022

View reviewed changes

zarend force-pushed the minimal-pseudo-checkbox branch from d41d563 to e89722a Compare November 18, 2022 16:51

zarend force-pushed the minimal-pseudo-checkbox branch 2 times, most recently from 18283be to 79a4c62 Compare November 18, 2022 17:35

devversion removed their request for review December 8, 2022 13:24

zarend removed the action: merge The PR is ready for merge by the caretaker label Jan 3, 2023

zarend force-pushed the minimal-pseudo-checkbox branch from 72ae1c8 to a83b959 Compare January 4, 2023 23:39

zarend requested a review from josephperrott as a code owner January 4, 2023 23:39

zarend force-pushed the minimal-pseudo-checkbox branch 4 times, most recently from 4cec738 to 613793e Compare January 5, 2023 17:09

zarend requested review from crisbeto and mmalerba January 5, 2023 17:15

zarend commented Jan 5, 2023

View reviewed changes

src/material/autocomplete/autocomplete.ts Outdated Show resolved Hide resolved

crisbeto reviewed Jan 5, 2023

View reviewed changes

zarend force-pushed the minimal-pseudo-checkbox branch 2 times, most recently from a71930e to fac89d6 Compare January 5, 2023 22:55

mmalerba approved these changes Jan 6, 2023

View reviewed changes

zarend added the action: merge The PR is ready for merge by the caretaker label Jan 9, 2023

zarend force-pushed the minimal-pseudo-checkbox branch from fac89d6 to 183b59a Compare January 11, 2023 23:44

zarend force-pushed the minimal-pseudo-checkbox branch from 183b59a to 0077201 Compare January 12, 2023 18:56

angular-robot bot merged commit 46cfbe5 into angular:main Jan 18, 2023

zarend mentioned this pull request Jan 27, 2023

docs(material/select, material/autocomplete): update a11y docs on che… #26515

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(material/core): add checkmark for single-select #25962

fix(material/core): add checkmark for single-select #25962

zarend commented Nov 11, 2022

crisbeto Nov 14, 2022

zarend Nov 14, 2022

crisbeto Nov 14, 2022

zarend Nov 15, 2022

zarend Nov 15, 2022

mmalerba Nov 18, 2022

zarend Jan 5, 2023

zarend commented Nov 17, 2022

github-actions bot commented Nov 17, 2022 •

edited

mmalerba left a comment

crisbeto left a comment

zarend commented Nov 18, 2022

zarend commented Jan 5, 2023

crisbeto Jan 5, 2023

zarend Jan 5, 2023

crisbeto Jan 5, 2023

zarend Jan 5, 2023

crisbeto Jan 5, 2023

mmalerba left a comment

zarend commented Jan 11, 2023 •

edited

angular-automatic-lock-bot bot commented Feb 18, 2023

		@@ -1,5 +1,6 @@
		<mat-pseudo-checkbox *ngIf="multiple" class="mat-mdc-option-pseudo-checkbox"

fix(material/core): add checkmark for single-select #25962

fix(material/core): add checkmark for single-select #25962

Conversation

zarend commented Nov 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarend commented Nov 17, 2022

github-actions bot commented Nov 17, 2022 • edited

mmalerba left a comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

zarend commented Nov 18, 2022

zarend commented Jan 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba left a comment

Choose a reason for hiding this comment

zarend commented Jan 11, 2023 • edited

angular-automatic-lock-bot bot commented Feb 18, 2023

github-actions bot commented Nov 17, 2022 •

edited

zarend commented Jan 11, 2023 •

edited