Skip to content

Accept new value when hitting ENTER to close a prompt dialog#12360

Merged
zsarnett merged 4 commits intohome-assistant:devfrom
Stormalong:fix-prompt-dialog
Apr 22, 2022
Merged

Accept new value when hitting ENTER to close a prompt dialog#12360
zsarnett merged 4 commits intohome-assistant:devfrom
Stormalong:fix-prompt-dialog

Conversation

@Stormalong
Copy link
Copy Markdown
Contributor

…essed

Proposed change

This fixes a bug that causes hitting ENTER to close a prompt dialog to not accept the new value.

Steps to reproduce:

  • go to Configuration
  • go to Devices & Services
  • click the kebab menu on any card and select Rename
  • type in a new name and hit ENTER (do not click OK)
  • the dialog will close but the name will not be changed

The existing code clearly intends for hitting ENTER (keycode 13) to accept the new value (by calling _confirm) but it doesn't work because the @change event isn't called for keystrokes.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

zsarnett
zsarnett previously approved these changes Apr 20, 2022
Copy link
Copy Markdown
Contributor

@zsarnett zsarnett left a comment

Choose a reason for hiding this comment

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

This will now call the value changed event each time a key is pressed..

@zsarnett zsarnett dismissed their stale review April 22, 2022 02:07

Meant to just comment

Stormalong and others added 3 commits April 22, 2022 12:01
Co-authored-by: Zack Barett <zackbarett@hey.com>
Co-authored-by: Zack Barett <zackbarett@hey.com>
@zsarnett zsarnett merged commit 269ef37 into home-assistant:dev Apr 22, 2022
Copy link
Copy Markdown
Member

@steverep steverep left a comment

Choose a reason for hiding this comment

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

I noticed this issue as well. Someone else reported in #12206. Per my comments, a better fix should be just to get rid of the key up and value changed handler methods, and grab the current field value in the confirm method, saving a bunch of lines of code.

@@ -77,7 +77,7 @@ class DialogBox extends LitElement {
dialogInitialFocus
.value=${this._value || ""}
@keyup=${this._handleKeyUp}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This handler is doing nothing because mwc-dialog hits the primary confirm button on enter key down, not up. The bug occurs because the confirm method then gets called before this._value is updated (because the change event only fires when focus is removed).

.value=${this._value || ""}
@keyup=${this._handleKeyUp}
@change=${this._valueChanged}
@input=${this._valueChanged}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not a good fix because it's just unnecessarily updating the state variable every time an input character is changed, and probably causing the lit element to keep rescheduling render updates.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants