feat(select): add alternate rendering strategy for improved accessibility by crisbeto · Pull Request #14430 · angular/components

crisbeto · 2018-12-08T13:01:38Z

Currently mat-select has some accessibility issues, because we don't keep the options in the DOM until the select is attached, making it harder for assistive technology to pick it up. These changes add an alternate, opt-in, rendering strategy that'll keep the options inside the DOM. The rendering strategy is controlled through the MAT_SELECT_RENDERING_STRATEGY injection token.

In order to facilitate the new rendering strategy, these changes introduce a new kind of portal called an InlinePortal. It is a portal whose content is transferred from one place in the DOM to another when it is attached/detached.

Another change that was necessary for the new rendering to work was being able to pass a portal to the CdkConnectedOverlay directive. If no portal is passed, the directive will fall back to the current behavior.

crisbeto · 2018-12-08T13:03:12Z

@jelbourn a few things that are up in the air:

Should we target a patch or a minor?
I wasn't too sure about the naming of some of the new symbols (e.g. "rendering strategy", "inline portal"). I'm open to different suggestions.
Should this be opt-in or opt-out? Also should it have an input on the MatSelect, in addition to the injection token?
Should we be running the mat-select tests both against the default rendering strategy and the inline one? mat-select is already the component with the most tests and this would double them. We'd probably see a bit of an increase in CI time.

ngbot · 2018-12-20T18:30:58Z

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

ngbot · 2018-12-20T18:31:16Z

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

jelbourn · 2019-01-08T21:52:27Z

Per open questions:

It should be minor since we're adding new APIs
It should be opt-in, since tons of people have unit tests that assume the select panel is absent when "closed"
We probably should do all the tests (except the ones that don't apply), even if it does increase the run time

jelbourn

High level- I don't quite follow why the new type of portal is necessary. What's the rundown of this approach vs. just visually the existing overlay?

jelbourn · 2019-01-08T22:05:35Z

+ }
+
+ /** DOM node hosting the portal's content. */
+ get origin(): HTMLElement {


jelbourn · 2019-01-08T22:33:43Z

+ [style.transformOrigin]="_transformOrigin"
+ [style.font-size.px]="_triggerFontSize"
+ (keydown)="_handleKeydown($event)">
+ <ng-content></ng-content>


Can we avoid doing the "ng-content inside ng-template` trick? It ends up always instantiating the content, even when it's not going to be attached.

We could make it work by allowing people to provide an ng-template as the content. The problem is that we need the content to be rendered, in order to know what the options inside the select are. We need to know the options for a lot of things like showing the selected value, knowing where to position the dropdown, supporting keyboard navigation while closed etc.

crisbeto · 2019-01-09T07:40:18Z

@jelbourn I'll go through the rest of the feedback. As for the reason why I introduced a new portal, it's because it makes it easier to switch between the two approaches and it's much more aligned with the rest of our setup. The alternative would be something like the approach in #11806 where we'd need to support multiple parallel code paths.

crisbeto · 2019-01-09T19:40:42Z

@jelbourn all the feedback should be addressed now.

crisbeto · 2019-01-12T14:23:00Z

@jelbourn the changes that we've discussed should be implemented now, can you take another look?

Adds a new type of portal called `DomPortal` which transfers the contents of a portal into the portal outlet and then restores them on destroy. This was implemented initially for angular#14430.

…lity Currently `mat-select` has some accessibility issues, because we don't keep the options in the DOM until the select is attached, making it harder for assistive technology to pick it up. These changes add an alternate, opt-in, rendering strategy that'll keep the options inside the DOM. The rendering strategy is controlled through the `MAT_SELECT_RENDERING_STRATEGY` injection token. In order to facilitate the new rendering strategy, these changes introduce a new kind of portal called an `InlinePortal`. It is a portal whose content is transferred from one place in the DOM to another when it is attached/detached. Another change that was necessary for the new rendering to work was being able to pass a portal to the `CdkConnectedOverlay` directive. If no portal is passed, the directive will fall back to the current behavior.

Adds a new type of portal called `DomPortal` which transfers the contents of a portal into the portal outlet and then restores them on destroy. This was implemented initially for angular#14430.

Splaktar · 2019-07-19T03:36:44Z

There is no mention here about presubmit failures or issues, but #11083 (comment) mentions

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.

@jelbourn has this been presubmitted again after the May 2019 updates? If so, can you please confirm that this needs to be considered for closing due to too many presubmit failures?

jelbourn · 2019-07-19T04:41:55Z

Right- this change was unfortunately too breaking to land for existing Google apps. The new plan is bake the improvements into the MDC-based select component.

Adds a new type of portal called `DomPortal` which transfers the contents of a portal into the portal outlet and then restores them on destroy. This was implemented initially for angular#14430.

Adds a new type of portal called `DomPortal` which transfers the contents of a portal into the portal outlet and then restores them on destroy. This was implemented initially for #14430.

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

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

crisbeto added the target: minor This PR is targeted for the next minor release label Dec 8, 2018

crisbeto assigned jelbourn Dec 8, 2018

crisbeto requested review from devversion, jelbourn and josephperrott as code owners December 8, 2018 13:01

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

crisbeto force-pushed the select-a11y-rework branch from 11e6b33 to 141c534Compare December 23, 2018 15:47

jelbourn reviewed Jan 8, 2019
View reviewed changes

crisbeto force-pushed the select-a11y-rework branch from 141c534 to cabdc2bCompare January 9, 2019 19:40

jelbourn reviewed Jan 9, 2019
View reviewed changes

Comment threadsrc/lib/select/select.html Outdated

crisbeto force-pushed the select-a11y-rework branch from cabdc2b to f3ffda0Compare January 9, 2019 20:17

crisbeto mentioned this pull request Feb 9, 2019
fix(input): input not focused on click in IE if clicked outside of the native input element #15108
Merged

jelbourn added the Accessibility This issue is related to accessibility (a11y) label Feb 26, 2019

crisbeto force-pushed the select-a11y-rework branch from f3ffda0 to 79bf5a9Compare March 9, 2019 18:49

crisbeto force-pushed the select-a11y-rework branch 2 times, most recently from 77879e3 to a2fa363Compare April 7, 2019 17:55

mmalerba added aaa and removed aaa labels Apr 25, 2019

Splaktar mentioned this pull request May 14, 2019
MatSelect accessibility rework #11083
Closed

crisbeto force-pushed the select-a11y-rework branch from a2fa363 to 223e2ffCompare May 23, 2019 18:48

crisbeto mentioned this pull request May 23, 2019
feat(portal): add new portal that projects DOM nodes #16101
Merged

crisbeto force-pushed the select-a11y-rework branch from 223e2ff to aeaf07dCompare May 24, 2019 05:36

Splaktar added the pr: needs rebase label Jul 19, 2019

jelbourn closed this Jul 19, 2019

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

Uh oh!

Conversation

crisbeto commented Dec 8, 2018

Uh oh!

crisbeto commented Dec 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngbotBot commented Dec 20, 2018

Uh oh!

ngbotBot commented Dec 20, 2018

Uh oh!

jelbourn commented Jan 8, 2019

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jelbournJan 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jelbournJan 8, 2019

Choose a reason for hiding this comment

Uh oh!

crisbetoJan 9, 2019

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jan 9, 2019

Uh oh!

crisbeto commented Jan 9, 2019

Uh oh!

Uh oh!

crisbeto commented Jan 12, 2019

Uh oh!

Splaktar commented Jul 19, 2019

Uh oh!

jelbourn commented Jul 19, 2019

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crisbeto commented Dec 8, 2018•
edited
Loading