fix(material/datepicker): update active date on focus #24172

zarend · 2022-01-07T00:15:02Z

When a date cell on the calendar receives focus, set the active date
to that cell. This ensures that the active date matches the date with
browser focus.

Previously, we set the active date on keydown and click, but that was
problematic for screen readers. That's because many screen readers trigger
a focus event instead of a keydown event when using screen reader
specific navigation (VoiceOver, Chromevox, NVDA).

Fixes #23483

src/material/datepicker/month-view.ts

zarend · 2022-01-07T18:35:19Z

src/material/datepicker/multi-year-view.ts

@@ -218,6 +218,21 @@ export class MatMultiYearView<D> implements AfterContentInit, OnDestroy {
    );
  }

+  _yearBecomesActive(event: MatCalendarUserEvent<number>) {


This is nearly identical to the _yearSelected method above. I'm open to suggestions on how to refactor this, or if we should refactor it to share code.

zarend · 2022-01-07T18:35:30Z

src/material/datepicker/year-view.ts

@@ -198,6 +198,25 @@ export class MatYearView<D> implements AfterContentInit, OnDestroy {
    );
  }

+  /** Handles when a new month becomes active. */


This is nearly identical to the _monthSelected method above. I'm open to suggestions on how to refactor this, or if we should refactor it to share code.

zarend · 2022-01-07T18:38:28Z

This is working and ready for review now. @jelbourn , is minor the correct target for this PR? It adds a public member to MatCalendarBody, but the change should be non-breaking.

github-actions · 2022-01-07T18:41:12Z

Deployed dev-app to: https://ng-comp-dev--pr-24172-194ec1b7aad7df4b3deed4820c983151-3fnykkj8.web.app

zarend · 2022-01-07T19:22:50Z

Looks like the focus is lost when pressing Page Up in the month view. I'm looking into that...

zarend · 2022-01-07T20:23:50Z

Looks like the focus is lost when pressing Page Up in the month view. I'm looking into that...

Fixed the losing focus problem.

zarend · 2022-01-07T20:38:09Z

Everything works when I test manually, so it appears to be a problem with the tests.

src/material/datepicker/calendar-body.ts

src/material/datepicker/month-view.ts

zarend · 2022-01-10T19:29:00Z

@crisbeto c0f8ecf
^ calling detectChanges seems to fix the failing tests.

After debugging this with console.log statements, I determined that the calendar body calls .focus on the active cell before receiving an updated value for activeCell. This causes it to focus on the old active cell, rather than the one set in the keydown handler.

Ensuring that change detection runs appears to be the fix here, but I'm curious why this wan't a problem until now.

zarend · 2022-01-10T21:10:25Z

Note that this is not going to fix the problem with VoiceOver navigation not updating the focused date until #24171 lands. That's because VoiceOver does not seem to trigger the focus event when navigating to a td element, but it does when navigating to a button element.

This PR and #24171 are safe to merge in either order, but #23483 will not be fixed until both PR's land.

src/material/datepicker/month-view.ts

jelbourn · 2022-01-10T21:18:06Z

src/material/datepicker/month-view.ts

@@ -329,6 +340,9 @@ export class MatMonthView<D> implements AfterContentInit, OnChanges, OnDestroy {
      this.activeDateChange.emit(this.activeDate);
    }

+    // Ensure the calendar body has the correct active cell.
+    this._changeDetectorRef.detectChanges();


Why is this necessary? This would invoke an entire extra change detection, which we generally want to avoid.

Yeah, this seems a little iffy. I'm guessing that it might be flushing this subscription: https://github.com/angular/components/blob/master/src/material/datepicker/calendar-body.ts#L201

After debugging this with console.log statements, I determined that the calendar body calls .focus on the active cell before receiving an updated value for activeCell. This causes it to focus on the old active cell, rather than the one set in the keydown handler.

Ensuring that change detection runs appears to be the fix here, but I'm curious why this wan't a problem until now.

If it works in real life but not in tests, it most likely means the test needs a detectChanges call and not the source code (which is very normal since in the test nothing is causing change dtections to happen autmatically)

I just pushed a change to only call detectChanges if the event is not trusted. This is only needed for the tests because fake keyboard events do not trigger change detection. I'm generally not a fan of having code that's only intended to run in the tests, and I'm curious how we usually handle this situation.

Calling detectChanges in the test code doesn't work because by that time the calendar body will have already triggered the activeDateChange event with a stale value. This wasn't a problem until now, because before focusing on a stale date wouldn't trigger the activeDateChange event to update the active date. Calling detectChanges in the test code would case the calendar body to focus again but on a updated value.

Here's a rough timeline of what happens in the unit tests if we don't call detectChanges when handling the keydown event.
0. (active Date is Jan 12 2021)

trigger a fake keyboard event with the right arrow key

MatMonthView sets this.activeDate to Jan 13 2021

MatMonthView calls _focusActiveDate

MatCalendarBody calls .focus on Jan 12, 2021 date then triggers an activeDateChange Event with Jan 12, 2021

test code calls detectChanges

month-view receives sets the active date to Jan 12, 2021

(active date should be Jan 13, 2021).

Is there a supported way to do this programmatically without dispatching fake events? If there isn't, teams will probably try dispatching keydown events manually to get this desired behavior. So if the support for this gets baked into the datepicker it will likely be depended on

Edit: Not saying that this is a bad thing, just that we should be aware that this might become the reality if we go this route

I'm not sure, another approach would be to call .focus() the date that the keyboard event targets, instead of updating activeDate in the month view. The month view would not set the activeDate itself, and would receive an updated value of the activeDate via the activeDateChange event.

i don't know how that would work with the page up/down arrow because they target dates which are not currently rendered, so there's nothing to call .focus on for them.

I'm generally not a fan of having code that's only intended to run in the tests, and I'm curious how we usually handle this situation.

This isn't really the way to think about this situation. In a real application, Angular will trigger change detection when a user interacts with the page (clicks, keypresses, etc). However, that won't happen if we trigger a fake event in a test (since obviously you can't trigger a real event in a test). So calling detectChanges in the test is the thing that most closely approximates what happens when the real app is running. The library code should almost never have unnecessary behaviors added to support tests (barring things like extra attrs we add for harnesses).

Okay, so I should think about if the behavior is different between the tests and a real application, rather than the exact code path.

The library code should almost never have unnecessary behaviors added to support tests

Does the library code mean Angular Material (this repo) or the Angular framework?

The above diff is out-of-date, but I made it only call detectChanges if the event is not trusted, so that we don't impact the performance in a real application.

josephperrott

LGTM

When a a date cell on the calendar recieves focus, set the active date to that cell. This ensures that the active date matches the date with browser focus. Previously, we set the active date on keydown and click, but that was problematic for screenreaders. That's because many screenreaders trigger a focus event instead of a keydown event when using screenreader specific navigation (VoiceOver, Chromevox, NVDA). Addresses angular#23483

zarend · 2022-01-15T00:01:41Z

@jelbourn I've responded to the PR comments and this is ready for your eyes again 👀 . Could you confirm that target: minor is appropriate for this PR? It makes an API change, but it does it in a non-breaking way. (cc @crisbeto also for Datepicker)

jelbourn · 2022-01-18T23:56:34Z

src/material/datepicker/month-view.ts

+    if (!event.isTrusted) {
+      // Manually triggered events in unit tests do not trigger change detection.
+      this._changeDetectorRef.detectChanges();
+    }


This isn't really the right approach to this issue. The unit test should directly call detectChanges- library code (this code) shouldn't add additional behaviors explosively to make unit tests pass. Having the tests call detectChanges in tandem with emitting the fake event is the normal/standard way of handling this.

Well, calling detectChanges in the unit test doesn't work in this situation because the microtask queue is flushing before change detection runs. the child component (MatCalendarBody) needs to received the updated data from this.activeDate before it can focus on the correct cell in this method

components/src/material/datepicker/calendar-body.ts

Line 209 in 194ec1b

_focusActiveCell(movePreview = true) {

.

Here's an explanation I wrote up earlier which seems to have gotten buried in the PR comments.

I just pushed a change to only call detectChanges if the event is not trusted. This is only needed for the tests because fake keyboard events do not trigger change detection. I'm generally not a fan of having code that's only intended to run in the tests, and I'm curious how we usually handle this situation.

Calling detectChanges in the test code doesn't work because by that time the calendar body will have already triggered the activeDateChange event with a stale value. This wasn't a problem until now, because before focusing on a stale date wouldn't trigger the activeDateChange event to update the active date. Calling detectChanges in the test code would case the calendar body to focus again but on a updated value.

Here's a rough timeline of what happens in the unit tests if we don't call detectChanges when handling the keydown event.
0. (active Date is Jan 12 2021)

trigger a fake keyboard event with the right arrow key

MatMonthView sets this.activeDate to Jan 13 2021

MatMonthView calls _focusActiveDate

MatCalendarBody calls .focus on Jan 12, 2021 date then triggers an activeDateChange Event with Jan 12, 2021

test code calls detectChanges

month-view receives sets the active date to Jan 12, 2021

(active date is Jan 12, but it should be Jan 13, 2021).

There is two approaches I can think of, but both required changing the library code (this code)

call detect changes in library code (this code) only if the event is not trusted

add a timeOut to _focusActiveCell so that the microtask queue does not get flushed until we manually flush it in the test code.

WDYT?

If it is too difficult to handle testing these in unit tests, I would suggest just test these cases in our e2e tests with real keyboard events

jelbourn · 2022-01-18T23:57:26Z

src/material/datepicker/year-view.ts

+    this.activeDateChange.emit(
+      this._dateAdapter.createDate(
+        this._dateAdapter.getYear(this.activeDate),
+        month,
+        Math.min(this._dateAdapter.getDate(this.activeDate), daysInMonth),
+      ),
+    );


Would the clampDate method on DateAdapter be a simpler way of implementing this?

crisbeto

LGTM assuming the last couple of comments will be resolved.

crisbeto · 2022-01-19T10:30:05Z

src/material/datepicker/calendar-body.ts

@@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
  /** Emits when a new value is selected. */
  @Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>();

+  /** Emits when a new date becomes active. */
+  @Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>();


nit: might be more accurate to call this activeDateChange to align with the events on the other calendar views.

zarend · 2022-01-21T20:50:20Z

Talked this over with @andrewseguin , This solution is problematic for testing because change detections needs to run before the microtask queue is flushed. We also looked into making MatCalendarBody.activeCell have a setter that calls _focusActiveDate, but that seemed to cause a regression with stealing the user's focus (see #11049).

Closing this PR, I'm looking into another approach, where we would be able to run change detection in the tests. Will send out a new PR when I have it working.

zarend · 2022-01-28T17:30:14Z

FYI this is closed in favor of #24279 24279

angular-automatic-lock-bot · 2022-02-28T00:03:27Z

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

zarend commented Jan 7, 2022

View reviewed changes

src/material/datepicker/month-view.ts Show resolved Hide resolved

zarend commented Jan 7, 2022

View reviewed changes

zarend requested review from devversion, crisbeto and jelbourn January 7, 2022 18:35

zarend added Accessibility This issue is related to accessibility (a11y) area: material/datepicker target: minor This PR is targeted for the next minor release dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Jan 7, 2022

zarend marked this pull request as ready for review January 7, 2022 18:36

zarend requested review from mmalerba and a team as code owners January 7, 2022 18:36

zarend mentioned this pull request Jan 7, 2022

Calendar navigation #24124

Closed

zarend requested a review from andrewseguin as a code owner January 7, 2022 20:18

zarend force-pushed the calendar-body-roving-tabindex branch from 2d6d48e to 906db9e Compare January 7, 2022 20:20

jelbourn reviewed Jan 7, 2022

View reviewed changes

src/material/datepicker/calendar-body.ts Outdated Show resolved Hide resolved

src/material/datepicker/month-view.ts Show resolved Hide resolved

zarend force-pushed the calendar-body-roving-tabindex branch from c0f8ecf to 9dc7040 Compare January 10, 2022 21:08

zarend force-pushed the calendar-body-roving-tabindex branch from 9dc7040 to 28d1cc0 Compare January 10, 2022 21:11

jelbourn reviewed Jan 10, 2022

View reviewed changes

josephperrott approved these changes Jan 11, 2022

View reviewed changes

zarend requested a review from jelbourn January 12, 2022 23:15

zarend force-pushed the calendar-body-roving-tabindex branch from e68ce66 to ddfd299 Compare January 14, 2022 22:57

zarend force-pushed the calendar-body-roving-tabindex branch 2 times, most recently from 42e7b8e to bb41ac8 Compare January 14, 2022 23:07

zarend force-pushed the calendar-body-roving-tabindex branch from bb41ac8 to 194ec1b Compare January 14, 2022 23:49

jelbourn reviewed Jan 18, 2022

View reviewed changes

crisbeto approved these changes Jan 19, 2022

View reviewed changes

zarend closed this Jan 21, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(material/datepicker): update active date on focus #24172

fix(material/datepicker): update active date on focus #24172

zarend commented Jan 7, 2022 •

edited by Splaktar

Loading

zarend Jan 7, 2022

zarend Jan 7, 2022

zarend commented Jan 7, 2022

github-actions bot commented Jan 7, 2022 •

edited

Loading

zarend commented Jan 7, 2022

zarend commented Jan 7, 2022

zarend commented Jan 7, 2022

zarend commented Jan 10, 2022

zarend commented Jan 10, 2022

jelbourn Jan 10, 2022

crisbeto Jan 11, 2022

zarend Jan 11, 2022

jelbourn Jan 12, 2022

zarend Jan 12, 2022

wagnermaciel Jan 12, 2022 •

edited

Loading

zarend Jan 12, 2022

jelbourn Jan 12, 2022

zarend Jan 12, 2022

josephperrott left a comment

zarend commented Jan 15, 2022

jelbourn Jan 18, 2022

zarend Jan 19, 2022

wagnermaciel Jan 19, 2022

jelbourn Jan 18, 2022

crisbeto left a comment

crisbeto Jan 19, 2022

zarend commented Jan 21, 2022

zarend commented Jan 28, 2022

angular-automatic-lock-bot bot commented Feb 28, 2022

fix(material/datepicker): update active date on focus #24172

fix(material/datepicker): update active date on focus #24172

Conversation

zarend commented Jan 7, 2022 • edited by Splaktar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarend commented Jan 7, 2022

github-actions bot commented Jan 7, 2022 • edited Loading

zarend commented Jan 7, 2022

zarend commented Jan 7, 2022

zarend commented Jan 7, 2022

zarend commented Jan 10, 2022

zarend commented Jan 10, 2022

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

wagnermaciel Jan 12, 2022 • edited Loading

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

josephperrott left a comment

Choose a reason for hiding this comment

zarend commented Jan 15, 2022

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

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarend commented Jan 21, 2022

zarend commented Jan 28, 2022

angular-automatic-lock-bot bot commented Feb 28, 2022

zarend commented Jan 7, 2022 •

edited by Splaktar

Loading

github-actions bot commented Jan 7, 2022 •

edited

Loading

wagnermaciel Jan 12, 2022 •

edited

Loading