refactor(toc): use cdkScrollable for scrolling, remove unnecessary inputs #321

willshowell · 2017-11-03T21:19:11Z

Combine duplicated logic
(try to) increase readability
Use cdkScrollable for subscribing to scroll events
Use _activeLinkIndex to ensure only a single link is active at a time (removes active from link interface)
Remove input decorator from links
Fix bug where navigating to a page with a fragment would not scroll the header into view until clicking on the link

Note: this still does not work for toc on the guides pages. I thought scrolling would be on the window and therefore be picked up by the scroll dispatcher, but that does not seem to be the case.

cc @jelbourn

jelbourn · 2017-11-13T20:41:34Z

src/app/shared/table-of-contents/table-of-contents.ts

-    if (target) {
-      target.scrollIntoView();
-    }
+  contentReady(): void {


I'd rather name this method for what it does rather than when it's called

jelbourn · 2017-11-13T20:43:29Z

src/app/shared/table-of-contents/table-of-contents.ts

-    }
+  /** Find the target from the url fragment and scroll it into view. */
+  private scrollFragmentIntoView(): void {
+    this._ngZone.runTask(() => {


Why does this need to enter the zone if it's only updating the scroll position?

Oops that was leftover from debugging something else. Removed.

jelbourn · 2017-11-13T20:44:55Z

src/app/shared/table-of-contents/table-of-contents.ts

+    this._ngZone.runTask(() => {
+      const target = document.getElementById(this._urlFragment);
+      if (target) {
+        // scrollIntoView may require next macrotask if target has just been loaded


I don't quite follow why this is necessary; the document-viewer knows exactly when the content is loaded and in the DOM

Without it, scroll position would always end up at the top of the page (can be seen here).

However I now recall that there was some other component that manually updates the scroll position. I'll try to track down any sort of race condition occurring there.

Yeah it's conflicting with the navigation focus directive,

material.angular.io/src/app/shared/navigation-focus/navigation-focus.ts

Lines 14 to 18 in f42d5f4

ngOnInit() {

clearTimeout(lastTimeoutId);

// 100ms timeout is used to allow the page to settle before moving focus for screen readers.

lastTimeoutId = setTimeout(() => this.el.nativeElement.focus(), 100);

}

EDIT: actually not so sure

EDIT 2: It was the component overview doing the auto focus, not the navigation focus directive

willshowell · 2017-11-13T22:11:03Z

@jelbourn ready for another review. The component overview and component api components were autofocusing the visually hidden element at the top after 100ms. I've changed them to wait until the content has loaded, but before the TOC scrolls the correct header into view.

Would it be better for a11y to place focus on the header link when the fragment does exist?

jelbourn · 2017-11-14T02:31:42Z

@willshowell Yeah, if there is a fragment then the autofocus should be on the header for that fragment. I think that was just an oversight when doing the autofocus. Would you be interested in addressing that in this PR?

willshowell · 2017-11-15T18:31:24Z

@jelbourn Ugh I thought I'd be able to easily, but I'm hitting some strange behavior where focus() is noop on the header-link unless setTimeout is used. I'd like to defer that to a later PR.

jelbourn · 2017-12-08T00:04:21Z

@amcdnl can you review?

amcdnl

Looks good overall, just one suggestion for lettables.

amcdnl · 2017-12-10T20:01:25Z

src/app/shared/table-of-contents/table-of-contents.ts

 import {Subject} from 'rxjs/Subject';
+import 'rxjs/add/operator/filter';


Lets use Rx Lettables here. I have a PR to transform the rest of them in the app.

…he toc.

… subscribing to events on container - Remove links as an input - Calculate container from scrollable event instead of using input

- Unfortunately seems to require setTimeout... Postponing to next microtask does not work (Promise.resolve)

willshowell · 2017-12-11T14:33:50Z

@jelbourn @amcdnl rebased and comments addressed

googlebot added the cla: yes label Nov 3, 2017

jelbourn reviewed Nov 13, 2017

View reviewed changes

willshowell force-pushed the refactor/toc branch from d748a50 to 5ad187b Compare November 13, 2017 22:07

jelbourn requested a review from amcdnl December 8, 2017 00:04

amcdnl reviewed Dec 10, 2017

View reviewed changes

willshowell added 9 commits December 11, 2017 08:20

Have link no longer maintain its own 'active' state. Handle that in t…

7f657ec

…he toc.

Refactor a little logic for cleaner readability

7941daa

Combine duplicated logic

bfe48dc

Rename method to be more descriptive

b36c688

Use scroll dispatcher to respond to scroll events instead of manually…

0cbecaf

… subscribing to events on container - Remove links as an input - Calculate container from scrollable event instead of using input

fix bug where scrollIntoView does not work on page load

86e9afa

- Unfortunately seems to require setTimeout... Postponing to next microtask does not work (Promise.resolve)

Address comments

0a0f46c

Ensure TOC scroll occurs after intial autofocus

57a3d76

Use lettables in TOC

0369155

willshowell force-pushed the refactor/toc branch from 5ad187b to 0369155 Compare December 11, 2017 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(toc): use cdkScrollable for scrolling, remove unnecessary inputs #321

refactor(toc): use cdkScrollable for scrolling, remove unnecessary inputs #321

willshowell commented Nov 3, 2017

jelbourn Nov 13, 2017

jelbourn Nov 13, 2017

willshowell Nov 13, 2017

jelbourn Nov 13, 2017

willshowell Nov 13, 2017

willshowell Nov 13, 2017 •

edited

Loading

willshowell commented Nov 13, 2017

jelbourn commented Nov 14, 2017

willshowell commented Nov 15, 2017

jelbourn commented Dec 8, 2017

amcdnl left a comment

amcdnl Dec 10, 2017

willshowell commented Dec 11, 2017

	ngOnInit() {
	clearTimeout(lastTimeoutId);
	// 100ms timeout is used to allow the page to settle before moving focus for screen readers.
	lastTimeoutId = setTimeout(() => this.el.nativeElement.focus(), 100);
	}

		import {Subject} from 'rxjs/Subject';
		import 'rxjs/add/operator/filter';

refactor(toc): use cdkScrollable for scrolling, remove unnecessary inputs #321

Are you sure you want to change the base?

refactor(toc): use cdkScrollable for scrolling, remove unnecessary inputs #321

Conversation

willshowell commented Nov 3, 2017

jelbourn Nov 13, 2017

Choose a reason for hiding this comment

jelbourn Nov 13, 2017

Choose a reason for hiding this comment

willshowell Nov 13, 2017

Choose a reason for hiding this comment

jelbourn Nov 13, 2017

Choose a reason for hiding this comment

willshowell Nov 13, 2017

Choose a reason for hiding this comment

willshowell Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

willshowell commented Nov 13, 2017

jelbourn commented Nov 14, 2017

willshowell commented Nov 15, 2017

jelbourn commented Dec 8, 2017

amcdnl left a comment

Choose a reason for hiding this comment

amcdnl Dec 10, 2017

Choose a reason for hiding this comment

willshowell commented Dec 11, 2017

willshowell Nov 13, 2017 •

edited

Loading