fix(select): Improve the a11y of select component by making overlay #11806

tinayuangao · 2018-06-15T20:51:03Z

always exist in the DOM

jelbourn · 2018-06-15T23:25:29Z

src/cdk/overlay/overlay-directives.ts

@@ -165,6 +165,9 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
  @Input('cdkConnectedOverlayScrollStrategy') scrollStrategy: ScrollStrategy =
      this._scrollStrategy();

+  /** Strategy to detach the overlay panel on ESCAPE keydown */
+  @Input('cdkConnectedOverlayDetachOnEscape') detachOnEscape: boolean = true;


I would invert this property so that it's just disableClose with a description:

/** Whether the overlay closes when the user pressed the Escape key. */

and also using coerceBooleanProperty

In that case the same check would have to be added to the backdropClick so it's consistent with all the other overlays.

@crisbeto backdropClick emits an event and developers decide what will happen after backdropClick. (Here in select we close the panel, but other things can be done)

jelbourn · 2018-06-15T23:29:28Z

src/lib/select/select.html

@@ -19,31 +19,31 @@
  cdk-connected-overlay
  cdkConnectedOverlayLockPosition
  cdkConnectedOverlayHasBackdrop
+  [cdkConnectedOverlayOpen]="true"


I don't think this needs the square braces any more

Also add a comment that explains that why the overlay is always "open"?

My concern with this one is how it would perform on a page with a lot of selects. I wonder whether we couldn't get away with not rendering it on init, but doing it when the user focuses on a select or hovers over it. That way we should have a split second to get it into the DOM before the user decides to open the panel.

Not mentioned here, but the second half of this change is going to be support the native <select> in mat-form-field for situations where mat-select is too heavy. The user can then decide to make the trade-off- of having the Material Design style panel or the much more performant native dropdown.

jelbourn · 2018-06-15T23:32:03Z

src/lib/select/select.spec.ts

@@ -396,6 +395,7 @@ describe('MatSelect', () => {
        it('should open the panel when pressing a vertical arrow key on a closed multiple select',
          fakeAsync(() => {
            fixture.destroy();
+            flush();


I'm worried that if we need to add flush in these tests, there will be a ton of downstream apps that will need to do the same

Could you elaborate on what makes these flush operations necessary now?

The flush is because something is run outside the angular zone when the overlay is attached: https://github.com/angular/material2/blob/master/src/cdk/overlay/overlay-ref.ts#L304

We can attach backdrop when the select panel is opened.

We can see how it affects google3 on presubmit

jelbourn · 2018-06-15T23:35:16Z

src/lib/select/select.ts

+    // Defer setting the value in order to void the "Expression has changed after it was checked"
+    // errors from Angular
+    Promise.resolve().then(() => {
+      this._initializePanel();


Does this have to happen in ngAfterContentInit, or can it happen once the panel is visually displayed?

@jelbourn How can we know the panel is visually displayed?

jelbourn · 2018-06-15T23:38:12Z

src/lib/select/select.ts

@@ -554,20 +563,32 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    this._keyManager.withHorizontalOrientation(null);
    this._calculateOverlayPosition();
    this._highlightCorrectOption();
+    this._showBackdrop();
+    this.overlayDir.overlayRef.overlayElement.style.minWidth = `${this._triggerRect.width}px`;


Why are setting this minWidth and the direction necessary now when they weren't before?

I'm also concerned about setting any geometry properties directly to the overlay panel, since that's supposed to be the responsibility of the overlay code itself.

jelbourn · 2018-06-15T23:42:17Z

src/lib/select/select.ts

+
+      this.overlayDir.overlayRef.updatePosition();
+
+      this._changeDetectorRef.detectChanges();


Why is the extra detectChanges necessary?

Otherwise we cannot calculate the correct scrollTop and offsetX values in the next steps

jelbourn · 2018-06-15T23:55:11Z

src/lib/select/select.ts

@@ -194,7 +195,7 @@ export class MatSelectTrigger {}
    '[attr.aria-required]': 'required.toString()',
    '[attr.aria-disabled]': 'disabled.toString()',
    '[attr.aria-invalid]': 'errorState',
-    '[attr.aria-owns]': 'panelOpen ? _optionIds : null',
+    '[attr.aria-owns]': '_optionIds',


Wasn't part of this change doing to make the select role="combobox"? That part is separate from where the options live, and I think it still reflects the interaction here more accurately.

Combobox requires a textbox inside it. I think it means there's an editable textbox and user may choose to enter something other than the list of options, which doesn't match our select component.

If #7835 ends up being merged, it would be nice to be able to use role="combobox" in correlation with the editable textbox that is focus-trapped in the mat-select-header.

The text input is optional for combobox; this prototype uses it without one:
https://stackblitz.com/edit/angular-a8j36n?file=app%2Fmat-select%2Fmat-select.component.html

As well as the deque select:
https://pattern-library.dequelabs.com/components/selects

In aria 1.0 it's not required and in aria 1.1 textbox or searchbox is required.

https://www.w3.org/TR/wai-aria-1.1/#combobox

Ah, interesting. We'll have to explore using combobox for a select with a search input in the future

crisbeto · 2018-06-16T07:42:50Z

src/cdk/overlay/overlay-directives.ts

@@ -165,6 +165,9 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
  @Input('cdkConnectedOverlayScrollStrategy') scrollStrategy: ScrollStrategy =
      this._scrollStrategy();

+  /** Strategy to detach the overlay panel on ESCAPE keydown */
+  @Input('cdkConnectedOverlayDetachOnEscape') detachOnEscape: boolean = true;


In that case the same check would have to be added to the backdropClick so it's consistent with all the other overlays.

crisbeto · 2018-06-16T07:44:09Z

src/lib/select/select.html

    [ngClass]="panelClass"
-    [@transformPanel]="multiple ? 'showing-multiple' : 'showing'"
+    [@transformPanel]="panelOpen ? (multiple ? 'showing-multiple' : 'showing') : 'void'"


Would be a good idea to move this into a function rather than having the nested ternary.

crisbeto · 2018-06-16T07:45:26Z

src/lib/select/select.spec.ts

@@ -396,6 +395,7 @@ describe('MatSelect', () => {
        it('should open the panel when pressing a vertical arrow key on a closed multiple select',
          fakeAsync(() => {
            fixture.destroy();


We should have a test that verifies that the select panel is in the DOM and with the correct amount of options on init.

crisbeto · 2018-06-16T07:47:35Z

src/lib/select/select.ts

@@ -758,6 +782,45 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    return !this._selectionModel || this._selectionModel.isEmpty();
  }

+  /** Show the backdrop of the overlay. */
+  private _showBackdrop() {


Not necessarily something for this PR, but rather than adding something special for the select, we can add the ability to detach and re-attach the backdrop via the OverlayRef.

I tried detach and re-attach the backdrop via the OverlayRef and I think there's a risk of detaching other overlay's backdrop accidentally or failed to detach correctly since there're some async activities outside angular zone. It's more reliable to make the backdrop visible/invisible.

If there's a risk that that detaching one overlay could unintentionally detach another, that sounds like a larger problem with the cdk or its implementation here.

I'm not sure I see how that could happen; each OverlayRef has a reference to its own specific backdrop element

Each OverlayRef has its own backdrop. The detach backdrop is an async call
this._ngZone.runOutsideAngular(() => setTimeout(finishDetach, 500));
When user click on the backdrop, it takes 500ms to fully detach the overlay. If in that 500ms user open the select panel again, a backdrop stilll exists so we cannot reattach it, and is removed after 500ms which means click outside the select panel will not close the panel.

There's transition.

backdropToDetach.classList.remove('cdk-overlay-backdrop-showing'); This triggers transition and finishDetach is triggered when the transitionend.
https://github.com/angular/material2/blob/master/src/cdk/overlay/overlay-ref.ts#L348

I see. So would you say the deeper issue here is that this._ngZone.runOutsideAngular(() => setTimeout(finishDetach, 500)); is meant to accommodate situations where there is no transitionend but currently it runs regardless of whether there's a transition or not?

What we were trying to avoid with that setTimeout is a call to getComputedStyle just so we can figure out whether the backdrop has a transition. I had a PR a while ago that switched the backdrop to be a component which would've given us better control over the animation, however we never managed to get it presubmitted (see #6627).

@crisbeto Do you think it would work better if, instead of having a 500ms timeout, there was a 10ms timeout that gets cleared with clearTimeout() if transitionstart is fired?

If I remember correctly, transitionstart was flaky between browsers which means that if we started depending on it, people's tests would start flaking as well.

crisbeto · 2018-06-16T07:49:23Z

src/lib/select/select.ts

+    this._changeDetectorRef.markForCheck();
+
+    // Set the font size on the panel element once it exists.
+    this._ngZone.onStable.asObservable().pipe(take(1)).subscribe(() => {


Rather than waiting for onStable and then having a null check for the overlayRef, we should be able to use the attach on the overlay directive to know when it was attached.

crisbeto · 2018-06-16T07:53:05Z

src/lib/select/select.html

@@ -19,31 +19,31 @@
  cdk-connected-overlay
  cdkConnectedOverlayLockPosition
  cdkConnectedOverlayHasBackdrop
+  [cdkConnectedOverlayOpen]="true"


My concern with this one is how it would perform on a page with a lot of selects. I wonder whether we couldn't get away with not rendering it on init, but doing it when the user focuses on a select or hovers over it. That way we should have a split second to get it into the DOM before the user decides to open the panel.

crisbeto · 2018-06-16T07:54:40Z

src/lib/select/select.ts

@@ -512,6 +513,14 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
      this._resetOptions();
      this._initializeSelection();
    });
+
+    this._triggerRect = this.trigger.nativeElement.getBoundingClientRect();


It seems like you're recalculating the _triggerRect here and right afterwards inside _initializePanel. We should be able to get away with only doing it in _initializePanel.

The trigger's bounding rectangle may change after the panel is initialized.
The panel is only initialized once when the component is loaded, but if the trigger's rect changed after that, we still need to get the correct scrollTop, offsetX, offsetY based on the new values.

tinayuangao · 2018-06-18T22:05:32Z

@crisbeto We discussed the possibility of loading options when user focus on the select, and we think it's too late.

jelbourn · 2018-06-20T18:32:57Z

Looks like there are just some lint errors now

googlebot · 2018-06-20T18:47:16Z

CLAs look good, thanks!

rafaelss95 · 2018-06-21T00:03:53Z

src/cdk/overlay/overlay-directives.ts

-  @Input('cdkConnectedOverlayOpen') open: boolean = false;
+  @Input('cdkConnectedOverlayOpen')
+  get open() { return this._open; }
+  set open(value: any) { this._open = coerceBooleanProperty(value); }


Just curious: any reason to not use boolean (in getter/setter)?

jelbourn

LGTM

jelbourn · 2018-06-21T21:56:24Z

Caretaker note: may need to change this from patch to something else depending on how many tests need to be updated in google

always exist in the DOM

ngbot · 2018-08-23T14:50:09Z

Hi @tinayuangao! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

Splaktar · 2018-10-05T21:39:34Z

Just wanted to link this over to the related issue: #11083.

Splaktar · 2019-07-19T03:30:13Z

@jelbourn I think that this one is a good candidate for closing or reviewing for closing at the next meeting?

It hasn't been stated here, but #11083 (comment) mentions that

Attempts at fixing this were all just too much of a breaking change, such that it was impossible to roll out to applications in Google already using MatSelect.

angular-automatic-lock-bot · 2019-09-11T02:03:36Z

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.}

tinayuangao requested review from crisbeto and jelbourn as code owners June 15, 2018 20:51

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 15, 2018

jelbourn reviewed Jun 16, 2018

View reviewed changes

crisbeto reviewed Jun 16, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jun 18, 2018

jrood mentioned this pull request Jun 19, 2018

feat(select): add mat-select-header component #7835

Closed

tinayuangao force-pushed the select-overlay2 branch from c14331f to dbecfa3 Compare June 20, 2018 18:47

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jun 20, 2018

rafaelss95 reviewed Jun 21, 2018

View reviewed changes

jrood mentioned this pull request Jun 21, 2018

refactor(overlay): use component to render backdrop #6627

Closed

jelbourn approved these changes Jun 21, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jun 21, 2018

This comment has been minimized.

Sign in to view

crisbeto mentioned this pull request Jul 11, 2018

Some MatSelect elements have an aria-labelledby referring to a non-existent ID #12157

Closed

fix(select): Improve the a11y of select component by making overlay

113997b

always exist in the DOM

jelbourn force-pushed the select-overlay2 branch from dbecfa3 to 113997b Compare July 11, 2018 22:31

jelbourn mentioned this pull request Jul 12, 2018

wip select a11y testing #12184

Closed

jelbourn mentioned this pull request Jul 27, 2018

perf(overlay): remove detached overlays from the DOM #12414

Merged

josephperrott added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 19, 2018

crisbeto mentioned this pull request Jan 9, 2019

feat(select): add alternate rendering strategy for improved accessibility #14430

Closed

mmalerba added aaa and removed aaa labels Apr 25, 2019

Splaktar added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jul 19, 2019

jelbourn closed this Jul 19, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019


		this.overlayDir.overlayRef.updatePosition();

		this._changeDetectorRef.detectChanges();

fix(select): Improve the a11y of select component by making overlay #11806

fix(select): Improve the a11y of select component by making overlay #11806

Conversation

tinayuangao commented Jun 15, 2018

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

tinayuangao commented Jun 18, 2018

jelbourn commented Jun 20, 2018

googlebot commented Jun 20, 2018

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Jun 21, 2018

This comment has been minimized.

ngbot bot commented Aug 23, 2018

Splaktar commented Oct 5, 2018

Splaktar commented Jul 19, 2019 • edited Loading

angular-automatic-lock-bot bot commented Sep 11, 2019

Splaktar commented Jul 19, 2019 •

edited

Loading