refactor(docs-infra): reimplement aio-select using mat-select #45937

dario-piotrowicz · 2022-05-09T20:26:21Z

the current aio-select component is implemented using buttons instead of
a native select element, this causes the component not to be very
accessible (screen readers for example do not tell the user that there
is a selection available, and not all keyboard navigation
functionalities work as per the wai-aria guidelines)

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

As mentioned in the commit description this is an attempt to make aio-select more accessible as I believe it currently is not

This is how it currently looks:

What is the new behavior?

This is how it looks with mat-select:

Does this PR introduce a breaking change?

Yes
No

Other information

I've checked the code and the only places using the component are the api-list page and the version select on the left sidenav
This is just an idea, since we've got material I figured we could try reusing mat-select, if the change in style is undesirable I can scratch the change and just manually re-implement the component using the native select element
This relates but does not completely addresses Accessibility issues with version picker #44339
Most of the aio-select unit tests have been removed since they test basic functionalities which should be guaranteed by the mat-select out of the box (like the fact that the options show up when you click on the element, etc...)
I think the mat-select has a slight issue with the a11y of the label, I've opened an issue for that in the components repo (bug(mat-select): The selection is repeated twice (+) in screen readers components#24899)

dario-piotrowicz · 2022-05-09T20:28:45Z

aio/src/app/shared/select/select.component.spec.ts

-    it('should contain a symbol if hasSymbol is true', () => {
-      expect(getButtonSymbol()).toEqual(null);
-      host.showSymbol = true;
-      fixture.detectChanges();
-      expect(getButtonSymbol()).not.toEqual(null);
-    });


This is one unit test I actually wanted to keep but I had issues implementing it since the span with the symbol is inside the mat-select and I couldn't find a way to query for it (I am not even sure if the mat-select content gets rendered in the tests) 😓

mary-poppins · 2022-05-13T11:24:42Z

You can preview e11d9f4 at https://pr45937-e11d9f4.ngbuilds.io/.

gkalpak

Generally lgtm 👍
I wonder what the payload size change would be.

jessicajaniuk

Looks like there's some api-list tests that relied on a CSS class that was changed here that needs to be addressed. That and the simple lint issue should get this ready to merge.

jessicajaniuk · 2022-05-13T17:34:09Z

aio/src/app/shared/select/select.component.spec.ts

+
+import { MatSelectHarness } from '@angular/material/select/testing';
+import { NoopAnimationsModule } from '@angular/platform-browser/animations';
+import { MatFormFieldHarness } from "@angular/material/form-field/testing";


Suggested change

import { MatFormFieldHarness } from "@angular/material/form-field/testing";

import { MatFormFieldHarness } from '@angular/material/form-field/testing';

Looks like aio tests are failing lint checks because of this.

cool yeah, sorry for the failing tests, I'm on it 🙂

dario-piotrowicz · 2022-05-13T17:52:08Z

Generally lgtm +1 I wonder what the payload size change would be.

yeah I was wondering the same too, I just thought of opening the PR and see what the ci says 🙈

let me fix the issues and will see how it looks 😅

the current aio-select component is implemented using buttons instead of a native select element, this causes the component not to be very accessible (screen readers for example do not tell the user that there is a selection available, and not all keyboard navigation functionalities work as per the wai-aria guidelines)

dario-piotrowicz · 2022-05-13T21:24:26Z

aio/tests/e2e/src/app.po.ts

-    await dropdown.element(by.css('.form-select-button')).click();
-    const menuItem = dropdown.element(by.cssContainingText('.form-select-dropdown li', itemName));
+    await dropdown.element(by.css('mat-select')).click();
+    const menuItem = element(by.cssContainingText('mat-option', itemName));


here I had to remove the dropdown scoping/prefix, the issue being that the select options sit in their cdk-overlay-container div which is not a child of the dropdown so I needed to make the selection generic

(PS: I looked into using the material harnesses but I think that they have issues with protractor, with it being deprecated and whatnot, please let me know if I'm mistaken 😓)

dario-piotrowicz · 2022-05-13T21:47:17Z

@gkalpak the results are out....

Sorry I had no idea it would cause such a drastic increase! 😓 😱

I'm just closing this PR and will create a new one using a native select element (in that way I can also keep the look and feel exactly the same)

@gkalpak and @jessicajaniuk thanks a lot for the reviewing the PR 🙂 , sorry for the inconvenience 😓

improve the accessibility of the aio-select component so that it is clear for screen reader users its functionality (currently it is presented as a simple button), following the WAI-ARIA authoring practices (see: https://www.w3.org/TR/wai-aria-practices/#combobox) A first attempt in improving the accessibility of the component has been tried in PR angular#45937 by replacing it with the material select component, such implementation has however been scrapped since the increase of payload sizes has proven prohibitively large (also note that given native select elements haven't been used given the lack of syling options for such elements)

improve the accessibility of the aio-select component so that it is clear for screen reader users its functionality (currently it is presented as a simple button), following the WAI-ARIA authoring practices (see: https://www.w3.org/TR/wai-aria-practices/#combobox) A first attempt in improving the accessibility of the component has been tried in PR #45937 by replacing it with the material select component, such implementation has however been scrapped since the increase of payload sizes has proven prohibitively large (also note that given native select elements haven't been used given the lack of syling options for such elements) PR Close #46013

angular-automatic-lock-bot · 2022-06-13T16:06:20Z

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

pullapprove bot requested a review from alxhub May 9, 2022 20:26

dario-piotrowicz commented May 9, 2022

View reviewed changes

AndrewKushnir requested review from gkalpak and removed request for alxhub May 9, 2022 22:47

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs-infra target: patch This PR is targeted for the next patch release labels May 9, 2022

ngbot bot modified the milestone: Backlog May 9, 2022

gkalpak closed this May 13, 2022

gkalpak reopened this May 13, 2022

gkalpak added type: bug/fix aio: preview labels May 13, 2022

gkalpak reviewed May 13, 2022

View reviewed changes

jessicajaniuk reviewed May 13, 2022

View reviewed changes

dario-piotrowicz added 2 commits May 13, 2022 22:00

fixup! refactor(docs-infra): reimplement aio-select using mat-select

420f5bb

dario-piotrowicz force-pushed the aio-select-refactor-to-use-mat-select branch from 260579f to 420f5bb Compare May 13, 2022 21:00

jessicajaniuk added the action: rerun CI at HEAD label May 13, 2022

ngbot bot removed the action: rerun CI at HEAD label May 13, 2022

dario-piotrowicz commented May 13, 2022

View reviewed changes

dario-piotrowicz closed this May 13, 2022

dario-piotrowicz deleted the aio-select-refactor-to-use-mat-select branch May 13, 2022 21:47

dario-piotrowicz mentioned this pull request May 16, 2022

fix(docs-infra): improve accessibility of aio-select component #46013

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(docs-infra): reimplement aio-select using mat-select #45937

refactor(docs-infra): reimplement aio-select using mat-select #45937

dario-piotrowicz commented May 9, 2022 •

edited

dario-piotrowicz May 9, 2022

mary-poppins commented May 13, 2022

gkalpak left a comment

jessicajaniuk left a comment

jessicajaniuk May 13, 2022

dario-piotrowicz May 13, 2022

dario-piotrowicz commented May 13, 2022

dario-piotrowicz May 13, 2022

dario-piotrowicz commented May 13, 2022 •

edited

angular-automatic-lock-bot bot commented Jun 13, 2022

	import { MatFormFieldHarness } from "@angular/material/form-field/testing";
	import { MatFormFieldHarness } from '@angular/material/form-field/testing';

refactor(docs-infra): reimplement aio-select using mat-select #45937

refactor(docs-infra): reimplement aio-select using mat-select #45937

Conversation

dario-piotrowicz commented May 9, 2022 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

dario-piotrowicz May 9, 2022

Choose a reason for hiding this comment

mary-poppins commented May 13, 2022

gkalpak left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

jessicajaniuk May 13, 2022

Choose a reason for hiding this comment

dario-piotrowicz May 13, 2022

Choose a reason for hiding this comment

dario-piotrowicz commented May 13, 2022

dario-piotrowicz May 13, 2022

Choose a reason for hiding this comment

dario-piotrowicz commented May 13, 2022 • edited

angular-automatic-lock-bot bot commented Jun 13, 2022

dario-piotrowicz commented May 9, 2022 •

edited

dario-piotrowicz commented May 13, 2022 •

edited