Skip to content

Conversation

@crisbeto
Copy link
Member

Fixes the case where the user might be holding down the enter key for a datepicker in touch mode, which could cause it to open multiple dialogs at the same time.

For reference:
demo

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Jul 16, 2018
@crisbeto crisbeto requested a review from mmalerba as a code owner July 16, 2018 20:15
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 16, 2018

/** Open the calendar as a dialog. */
private _openAsDialog(): void {
if (this._dialogRef) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, why is this needed? It looks like in the open method where this is called from we already set a variable to make sure it only gets opened once. Is that not working for some reason?

Copy link
Member Author

@crisbeto crisbeto Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that when you're holding the key down, eventually some of them go through, because we reset some of the variables in a setTimeout. You can see in the gif how it can take a little bit of holding before the dialogs start stacking.

EDIT: I think it might also have to do with the fact that we change _opened variable in a callback to dialogRef.afterClosed which fires once the animation is finished.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change feels like masking the problem rather than fixing it. What if we switch to doing it in beforeClosed instead?

Copy link
Member Author

@crisbeto crisbeto Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one doesn't help much either. The only difference is that beforeClose is fired when the animation starts (which is still async) and the other one fires when the animation is done. IMO doing this is fine since it's pretty low maintenance on our end and it handles an edge case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, as long as we've tried other solutions. Can you add a comment explaining why this is necessary even though it looks like it would be handled above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Fixes the case where the user might be holding down the enter key for a datepicker in touch mode, which could cause it to open multiple dialogs at the same time.
@crisbeto crisbeto force-pushed the datepicker-multiple-dialog branch from b5e88ae to ad9a032 Compare July 18, 2018 05:12
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 18, 2018
@jelbourn jelbourn merged commit 8e63656 into angular:master Aug 23, 2018
jelbourn pushed a commit that referenced this pull request Aug 29, 2018
#12238)

Fixes the case where the user might be holding down the enter key for a datepicker in touch mode, which could cause it to open multiple dialogs at the same time.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants