Skip to content

fix(popover-edit): unable to close by pressing enter on some screen readers #16466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 0 additions & 37 deletions src/cdk-experimental/popover-edit/edit-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,6 @@ export class EditRef<FormValue> implements OnDestroy {
/** The value to set the form back to on revert. */
private _revertFormValue: FormValue;

/**
* The flags are used to track whether a keyboard enter press is in progress at the same time
* as other events that would cause the edit lens to close. We must track this so that the
* Enter keyup event does not fire after we close as it would cause the edit to immediately
* reopen.
*/
private _enterPressed = false;
private _closePending = false;

constructor(
@Self() private readonly _form: ControlContainer,
private readonly _editEventDispatcher: EditEventDispatcher,
Expand Down Expand Up @@ -88,34 +79,6 @@ export class EditRef<FormValue> implements OnDestroy {
this._blurredSubject.next();
}

/**
* Closes the edit if the enter key is not down.
* Otherwise, sets _closePending to true so that the edit will close on the
* next enter keyup.
*/
closeAfterEnterKeypress(): void {
// If the enter key is currently pressed, delay closing the popup so that
// the keyUp event does not cause it to immediately reopen.
if (this._enterPressed) {
this._closePending = true;
} else {
this.close();
}
}

/**
* Called on Enter keyup/keydown.
* Closes the edit if pending. Otherwise just updates _enterPressed.
*/
trackEnterPressForClose(pressed: boolean): void {
if (this._closePending) {
this.close();
return;
}

this._enterPressed = pressed;
}

/**
* Resets the form value to the specified value or the previously set
* revert value.
Expand Down
24 changes: 7 additions & 17 deletions src/cdk-experimental/popover-edit/lens-directives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export class CdkEditControl<FormValue> implements OnDestroy, OnInit {
if (this.ignoreSubmitUnlessValid && !this.editRef.isValid()) { return; }

this.editRef.updateRevertValue();
this.editRef.closeAfterEnterKeypress();
this.editRef.close();
}

/** Called on Escape keyup. Closes the edit. */
Expand Down Expand Up @@ -128,21 +128,9 @@ export class CdkEditControl<FormValue> implements OnDestroy, OnInit {
// to avoid double event listeners, we need to use `HostListener`. Once Ivy is the default, we
// can move this back into `host`.
// tslint:disable:no-host-decorator-in-concrete
@HostListener('keydown')
_handleKeydown() {
this.editRef.trackEnterPressForClose(true);
}

// In Ivy the `host` metadata will be merged, whereas in ViewEngine it is overridden. In order
// to avoid double event listeners, we need to use `HostListener`. Once Ivy is the default, we
// can move this back into `host`.
// tslint:disable:no-host-decorator-in-concrete
@HostListener('keyup', ['$event'])
_handleKeyup(event: KeyboardEvent) {
// TODO(crisbeto): should use cdk/keycodes once the tests are reworked to use cdk/testing.
if (event.key === 'Enter') {
this.editRef.trackEnterPressForClose(false);
} else if (event.key === 'Escape') {
@HostListener('keydown', ['$event'])
_handleKeydown(event: KeyboardEvent) {
if (event.key === 'Escape') {
this.close();
}
}
Expand Down Expand Up @@ -204,6 +192,8 @@ export class CdkEditClose<FormValue> {
// tslint:disable:no-host-decorator-in-concrete
@HostListener('click')
closeEdit(): void {
this.editRef.closeAfterEnterKeypress();
// Note that we use `click` here, rather than a keyboard event, because some screen readers
// will emit a fake click event instead of an enter keyboard event on buttons.
this.editRef.close();
}
}
4 changes: 2 additions & 2 deletions src/cdk-experimental/popover-edit/popover-edit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ abstract class BaseTestComponent {
openLens(rowIndex = 0, cellIndex = 1) {
this.focusEditCell(rowIndex, cellIndex);
this.getEditCell(rowIndex, cellIndex)
.dispatchEvent(new KeyboardEvent('keyup', {bubbles: true, key: 'Enter'}));
.dispatchEvent(new KeyboardEvent('keydown', {bubbles: true, key: 'Enter'}));
flush();
}

Expand Down Expand Up @@ -772,7 +772,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
component.openLens();

component.getNameInput()!.dispatchEvent(
new KeyboardEvent('keyup', {bubbles: true, key: 'Escape'}));
new KeyboardEvent('keydown', {bubbles: true, key: 'Escape'}));

expect(component.lensIsOpen()).toBe(false);
clearLeftoverTimers();
Expand Down
11 changes: 9 additions & 2 deletions src/cdk-experimental/popover-edit/table-directives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export class CdkEditable implements AfterViewInit, OnDestroy {
share(),
).subscribe(this.editEventDispatcher.allRows);

fromEvent<KeyboardEvent>(element, 'keyup').pipe(
fromEvent<KeyboardEvent>(element, 'keydown').pipe(
filter(event => event.key === 'Enter'),
toClosest(CELL_SELECTOR),
takeUntil(this.destroyed),
Expand Down Expand Up @@ -276,7 +276,14 @@ export class CdkPopoverEdit<C> implements AfterViewInit, OnDestroy {
this.template!,
this.viewContainerRef,
{$implicit: this.context}));
this.focusTrap!.focusInitialElement();

// We have to defer trapping focus, because doing so too early can cause the form inside
// the overlay to be submitted immediately if it was opened on an Enter keydown event.
this.services.ngZone.runOutsideAngular(() => {
setTimeout(() => {
this.focusTrap!.focusInitialElement();
});
});

// Update the size of the popup initially and on subsequent changes to
// scroll position and viewport size.
Expand Down
4 changes: 2 additions & 2 deletions src/material-experimental/popover-edit/popover-edit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ abstract class BaseTestComponent {
openLens(rowIndex = 0, cellIndex = 1) {
this.focusEditCell(rowIndex, cellIndex);
this.getEditCell(rowIndex, cellIndex)
.dispatchEvent(new KeyboardEvent('keyup', {bubbles: true, key: 'Enter'}));
.dispatchEvent(new KeyboardEvent('keydown', {bubbles: true, key: 'Enter'}));
flush();
}

Expand Down Expand Up @@ -691,7 +691,7 @@ matPopoverEditTabOut`, fakeAsync(() => {
component.openLens();

component.getInput()!.dispatchEvent(
new KeyboardEvent('keyup', {bubbles: true, key: 'Escape'}));
new KeyboardEvent('keydown', {bubbles: true, key: 'Escape'}));

expect(component.lensIsOpen()).toBe(false);
clearLeftoverTimers();
Expand Down