fix(dialog): restore focus with the proper focus origin

[devversion]

• Restores the trigger focus upon dialog close with the proper focus origin that caused the dialog closing.

For example, a backdrop click leads to a focus restore via mouse. Pressing escape leads to a focus restore via keyboard.

References #8420

[devversion]

Addressed the feedback and also added the check for the matDialogClose we discussed in the standup.

[jelbourn]

LGTM

[mmalerba]

@devversion please rebase

[devversion]

@mmalerba Done.

[ngbot]

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

[ngbot]

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

[devversion]

@josephperrott Rebased that one. Can we retry a presubmit in the future?

[devversion]

@jelbourn @josephperrott I rebased this one again. should we proceed with that one or don't you think it's worth moving forward with that? It definitely seems like a reasonable a11y feature for the dialog to me.

[zelliott]

Just a quick bump that it would be awesome to get this in.

[devversion]

Rebased again. If we decide to move forward with this, then the changes should most likely also be ported over to the cdk-experimental/dialog version.

[andrewseguin]

@devversion Want to move forward with this PR? If so, this is our next oldest P2 and I'm going to make it my side project to get a green presubmit with it.

[devversion]

@andrewseguin That would be awesome. I've rebased the PR. Let me know if I can help somehow!

[andrewseguin]

We're at a fork in the roads on this PR. This breaks a lot of internal tests.

Many tests provide a fake dialog ref using jasmine, e.g.

const dialogRefSpy = jasmine.createSpyObj('MatDialogRef', ['close']);

Then in their test, they check that close was called:

expect(dialogRefSpy.close).toHaveBeenCalledWith(false);

This change breaks that because now MatDialogClose calls _closeVia instead of close, so their expectation fails.

The "right thing to do" is to provide a fake for users so they don't need to make their own. Then they can just verify through the fake that expect(fakeDialogRef.closeResult).toBe(expectation). However, there are 1000+ instances of this internally, and it's going to take so many hours/days dedicated to migrate the tests.

An alternative path forward is to change this code a bit so that (click) still calls close(), but first sets some state through the MatDialogRef. For example, you can change _onButtonClick to be:

_onButtonClick(event: MouseEvent) {
    // Determinate the focus origin using the click event, because using the FocusMonitor will
    // result in incorrect origins. Most of the time, close buttons will be auto focused in the
    // dialog, and therefore clicking the button won't result in a focus change. This means that
    // the FocusMonitor won't detect any origin change, and will always output `program`.
    // [Plus additional comment on why we are setting this directly into the ref]
    this.dialogRef._closeInteractionType = event.screenX === 0 && event.screenY === 0 ? 'keyboard' : 'mouse';
    this.dialogRef.close(this.dialogResult);
  }

Then during close, the dialogRef would pass along that _closeInteractionType onto the container (or just do it from this function). This way, the API is preserved and all tests in Google would pass since close is still being called directly from the close directive.

This is obviously not ideal or best practice to be setting some state before calling a function, but a comment could be provided to explain why this is the case, as well as an additional test that makes sure we don't regress.

I still think it would be valuable to provide a fake to our users, but at least this won't be blocked on that migration occurring, which could possibly never get prioritized.

[devversion]

@andrewseguin Thanks for looking into this. I've made the changes as discussed. Instead of manually assigning _closeInteractionType all the time, I've created an internal helper function that takes care of this. This will also allow us to easily remove this logic in the future if we ever sort these dialog ref mock complications out.

[angular-automatic-lock-bot]

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