Skip to content
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

fix(lemon): remove buggy delay from _onBlur for LemonInputSelect #26400

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

havenbarnes
Copy link
Contributor

@havenbarnes havenbarnes commented Nov 25, 2024

Problem

Addresses issue described here: https://posthoghelp.zendesk.com/agent/tickets/20603

The problem: When clicking on an item from the popover which does not match the text input, the delayed _onBlur typically runs after the _onActionItem, reverting the selection back as can be seen in the Loom
Screenshot 2024-11-25 at 11 22 48 AM

Note - currently with the delay, this is a race condition. However when we remove the delay, the click action runs the _onActionItem, and because the popover is present during the click, the _onBlur will then go into its early return condition if (popoverFocusRef.current) {, which closes the popover, focuses the text input, and does not try to set any state values, which feels like the exact desired behavior

Given the deleted comment, I also tested all sorts of clicks in or around the popover, opening other popovers, etc and I can not find any adverse effects of removing this delay, but I'm wondering if @benjackwhite has any thoughts here as he added the delay initially when creating this component!

Changes

Remove the setTimeout wrapping _onBlur for LemonInputSelect

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

https://www.loom.com/share/ec54ef15771749b98d5ae11354cbb496

@havenbarnes
Copy link
Contributor Author

@benjackwhite I know it's a deep cut from a while ago but curious if you remember any context around // We need to add a delay as a click could be in the popover or the input wrapper which refocuses 😃

Copy link
Contributor

github-actions bot commented Nov 25, 2024

Size Change: 0 B

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.11 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

Seems fine and safe to me, but good call asking Ben for original context, and I'd wait to hear from him before shipping if this can wait a few days – it doesn't look like this is urgently affecting the customer, so we should be okay to be a bit cautious.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes self-requested a review November 27, 2024 19:12
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Oooh i hope this doesn't break anything... It was too long ago for me to know why this mattered but be aware if it does break it might be a very obscur change so please test heavily

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

I remember which case this change affects negatively. With the delay, clicking away from the LemonInputSelect immediately adds the current input as a value, see on master:

good

This is no longer true on this branch. Without the delay, two clicks are needed, which is not that smooth:

not good

@Twixes
Copy link
Collaborator

Twixes commented Nov 28, 2024

I think this is fine though

@havenbarnes
Copy link
Contributor Author

I remember which case this change affects negatively. With the delay, clicking away from the LemonInputSelect immediately adds the current input as a value, see on master:

good good

This is no longer true on this branch. Without the delay, two clicks are needed, which is not that smooth:

not good not good

That's interesting thanks for the callout! I feel like since there's an explicit "Add <typed_text>` button, this is a pretty good tradeoff as-is; losing one bit of convenience, but fixing a quite annoying and finicky bug. Regardless, I'll timebox and see if I can achieve both!

@havenbarnes
Copy link
Contributor Author

@Twixes thanks for the callout there - was able to get the "click outside popover adds custom value" functionality back as well: https://www.loom.com/share/ec54ef15771749b98d5ae11354cbb496 !

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Perfect 🙌

@Twixes Twixes enabled auto-merge (squash) December 3, 2024 14:10
@Twixes Twixes merged commit bcbe783 into master Dec 3, 2024
96 checks passed
@Twixes Twixes deleted the fix/release-condition-autofill branch December 3, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants