feat(typeahead): accessibility, options summary status message #2197

ymeine · 2018-02-28T17:47:56Z

Closes #1945

pkozlowski-opensource · 2018-03-16T13:44:46Z

src/util/accessibility/live.ts

+
+  ngOnDestroy() { this._element.remove(); }
+
+  async say(message: string): Promise<void> {


Could you please not use async-await for now? It transpiles to rather verbose code.

More important - why do we return a Promise here? We don't care about any return value, do we? What would be the use-case for knowing when announcing starts?

I think this is pity to deprive ourselves of one of the best standard language features that was brought these last years: now asynchronous code remains explicit and controlled, but is aligned with synchronous code. It's what made me adopt async code widely, because otherwise dealing with Promises chaining is quite verbose.

This is a general comment, and of course here using a Promise is easy, since I even had to create one due to the lack of proper API for creating a delay.

However:

first, the code size shouldn't be an issue, especially once compressed

then, we should configure the compilation so that it doesn't generate the helpers in each file, but instead relies on the TypeScript library to have this piece of code only once.

last, we don't really care about the return value here. It's a Promise because async functions return Promises.

pkozlowski-opensource · 2018-03-16T13:45:55Z

src/util/accessibility/live.ts

+
+    element.classList.add('sr-only');
+
+    document.body.appendChild(element);


Who is going to remove this element? We should have onDestroy, maybe?

Oh, actually it is here, never mind

pkozlowski-opensource · 2018-03-16T13:49:15Z

src/typeahead/typeahead.ts

@@ -357,9 +358,15 @@ export class NgbTypeahead implements ControlValueAccessor,
        // the change detection turn wouldn't be invoked automatically.
        this._windowRef.changeDetectorRef.detectChanges();
      }
+
+      this._live.say(this._srOptionsSummaryTemplate({count: results.length}));


Why do you pass an object here instead of just a number? It results in the unnecessary object creation and you read only count from it anyway.

or no need in method at all

I don't know why this appears as an outdated comment, but I'll reply anyways

No need for that indeed. I started with a public API, so I made it generic:

users would give a callback, which is now the private method we can see

I would give them an object since this is more future-proof in case we want to give more information

I kept this design while making things private.

However I would not be concerned AT ALL with performances. These are micro-optimizations, and I think you are well placed to know that actual performance is more complex than that. In this context, the feature call rate is way too low to worry about extra nanoseconds spent creating an object, calling a method and reading from this object. My concerns would be more about maintainability. But here I can remove both the method and the object as long as it remains private, it's a matter of one line of code so it's perfectly fine.

pkozlowski-opensource · 2018-03-16T13:52:17Z

src/util/accessibility/live.ts

+    const element = this._element;
+
+    element.textContent = '';
+    await new Promise(resolve => setTimeout(resolve, 100));


Why 100? How was this number chosen? Why does it need to be async in the first place? As a very minimum we need documentation that explain this.

This is all explained there: material2/live-announcer.ts at 6405da9b8e8532a7e5c854c920ee1815c275d734 · angular/material2.

It seems natural to wait a bit in case, knowing the unpredictability of screen readers. However, choosing a proper amount of delay is purely arbitrary.

maxokorokov

Ok for introducing the announcer.

checked with NVDA and JAWS 17, seems to work.
typeahead is broken for Voiceover, but not because of the announcer

I think we should find a way to remove delay from tests, make it customizeable?

maxokorokov · 2018-03-21T09:54:34Z

src/typeahead/typeahead.spec.ts

@@ -513,6 +513,7 @@ describe('ngb-typeahead', () => {
         changeInput(compiled, 'o');
         fixture.detectChanges();
         tick(250);
+         tick(100);  // for the aria-live announcer


I think we should use a different announcer for tests or inject delay value 0 so it would become synchronous

ANYTHING to get rid of these weird fake things like tick would please me. I'll explore the injection of a different delay, seems the simplest and most flexible, that could even be useful for users.

maxokorokov · 2018-03-21T09:55:39Z

src/typeahead/typeahead.ts

@@ -357,9 +358,15 @@ export class NgbTypeahead implements ControlValueAccessor,
        // the change detection turn wouldn't be invoked automatically.
        this._windowRef.changeDetectorRef.detectChanges();
      }
+
+      this._live.say(this._srOptionsSummaryTemplate({count: results.length}));


or no need in method at all

maxokorokov · 2018-03-21T09:56:34Z

src/util/accessibility/live.ts

+export class Live implements OnDestroy {
+  private _element: HTMLElement;
+
+  constructor(@Inject(DOCUMENT) private _document: any) { this._element = this._createElement(); }


nitpick: no need to store document privately

Why would I make it public? It's an internal thing, it's not part of the API. Otherwise this removes the whole purpose of having visibility specifiers in the typing system.

ymeine · 2018-04-06T16:18:00Z

Changes done.

maxokorokov

Thanks, LGTM, just 2 things:

fix failing tests in IE
rename DELAY and related types/constants → something more precise

Will merge after fixes

maxokorokov · 2018-04-17T09:35:09Z

src/util/accessibility/live.ts

+    }
+  }
+
+  private _createElement(document): HTMLElement {


nitpick: this doesn't have to be private, can be a static outside function

maxokorokov · 2018-04-17T09:39:19Z

src/util/accessibility/live.ts

+
+// usefulness (and default value) of delay documented in Material's CDK
+// https://github.com/angular/material2/blob/6405da9b8e8532a7e5c854c920ee1815c275d734/src/cdk/a11y/live-announcer/live-announcer.ts#L50
+export type DELAY_TYPE = number | null;


Could we name them more precisely, please? {provide: DELAY, ...} - not clear which delay.

Maybe something like ARIA_LIVE_DELAY ?

maxokorokov · 2018-04-17T09:40:01Z

src/util/accessibility/live.ts

+    this._element = this._createElement(document);
+  }
+
+  ngOnDestroy() { this._element.remove(); }


Apparently .remove() is failing in IE 10-11 tests

maxokorokov · 2018-04-17T09:50:00Z

src/typeahead/typeahead.ts

@@ -354,6 +355,8 @@ export class NgbTypeahead implements ControlValueAccessor,

        this._showHint();
      }
+      const count = results.length;
+      this._live.say(count === 0 ? 'No result available' : `${count} options available`);


0 → No results available
1 → 1 result available
n → n results available

maxokorokov · 2018-04-23T11:52:18Z

@ymeine, could you please fix a failing test and I'll merge:

Firefox 59.0.0 (Linux 0.0.0) LiveAnnouncer live announcer should remove the used element from the DOM on destroy FAILED
	TypeError: this._element.parentElement is null in node_modules/systemjs/dist/system.src.js line 1556 > eval (line 32)
	Live.prototype.ngOnDestroy@src/util/accessibility/live.ts:37:18 <- temp/util/accessibility/live.js:32:48
	@src/util/accessibility/live.spec.ts:33:20 <- temp/util/accessibility/live.spec.js:34:40

closes ng-bootstrap#1945

pkozlowski-opensource added needs: review type: accessibility component: typeahead labels Mar 2, 2018

pkozlowski-opensource added the type: feature label Mar 16, 2018

pkozlowski-opensource reviewed Mar 16, 2018

View reviewed changes

pkozlowski-opensource added needs: work and removed needs: review labels Mar 16, 2018

maxokorokov requested changes Mar 21, 2018

View reviewed changes

ymeine force-pushed the pr/feat/accessibility/1945 branch from fb864d6 to d3cfdd2 Compare March 29, 2018 14:09

ymeine force-pushed the pr/feat/accessibility/1945 branch from d3cfdd2 to cf83d84 Compare April 6, 2018 16:17

ymeine force-pushed the pr/feat/accessibility/1945 branch from cf83d84 to 855a31f Compare April 6, 2018 16:20

pkozlowski-opensource added needs: review and removed needs: work labels Apr 13, 2018

pkozlowski-opensource added this to the 2.1.0 milestone Apr 13, 2018

ymeine force-pushed the pr/feat/accessibility/1945 branch from 855a31f to c156951 Compare April 13, 2018 17:16

maxokorokov self-assigned this Apr 17, 2018

maxokorokov requested changes Apr 17, 2018

View reviewed changes

maxokorokov added needs: work and removed needs: review labels Apr 17, 2018

maxokorokov reviewed Apr 17, 2018

View reviewed changes

ymeine force-pushed the pr/feat/accessibility/1945 branch 2 times, most recently from dc8be3e to f39aec9 Compare April 20, 2018 10:16

ymeine force-pushed the pr/feat/accessibility/1945 branch from f39aec9 to 2f2ad4e Compare April 27, 2018 14:01

feat(typeahead): accessibility, options summary status message

060693f

closes ng-bootstrap#1945

ymeine force-pushed the pr/feat/accessibility/1945 branch from 2f2ad4e to 060693f Compare April 27, 2018 15:14

maxokorokov approved these changes Apr 28, 2018

View reviewed changes

maxokorokov removed the needs: work label Apr 28, 2018

maxokorokov merged commit f65b8a4 into ng-bootstrap:master Apr 28, 2018

maxokorokov modified the milestones: 2.1.0, 2.0.0 Apr 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typeahead): accessibility, options summary status message #2197

feat(typeahead): accessibility, options summary status message #2197

ymeine commented Feb 28, 2018

pkozlowski-opensource Mar 16, 2018

pkozlowski-opensource Mar 16, 2018

ymeine Mar 30, 2018

pkozlowski-opensource Mar 16, 2018

pkozlowski-opensource Mar 16, 2018

pkozlowski-opensource Mar 16, 2018

maxokorokov Mar 21, 2018

ymeine Mar 30, 2018

pkozlowski-opensource Mar 16, 2018

ymeine Mar 30, 2018

maxokorokov left a comment

maxokorokov Mar 21, 2018

ymeine Mar 30, 2018

maxokorokov Mar 21, 2018

maxokorokov Mar 21, 2018

ymeine Mar 30, 2018

ymeine commented Apr 6, 2018

maxokorokov left a comment

maxokorokov Apr 17, 2018

maxokorokov Apr 17, 2018

maxokorokov Apr 17, 2018

maxokorokov Apr 17, 2018

maxokorokov commented Apr 23, 2018


		ngOnDestroy() { this._element.remove(); }

		async say(message: string): Promise<void> {


		element.classList.add('sr-only');

		document.body.appendChild(element);

feat(typeahead): accessibility, options summary status message #2197

feat(typeahead): accessibility, options summary status message #2197

Conversation

ymeine commented Feb 28, 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

maxokorokov left a 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

ymeine commented Apr 6, 2018

maxokorokov left a 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

maxokorokov commented Apr 23, 2018