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

Dismissing dialog after editing selection in async combobox loses hidden value #180

Closed
ILiuz opened this issue Jun 21, 2024 · 3 comments · Fixed by #191
Closed

Dismissing dialog after editing selection in async combobox loses hidden value #180

ILiuz opened this issue Jun 21, 2024 · 3 comments · Fixed by #191

Comments

@ILiuz
Copy link
Contributor

ILiuz commented Jun 21, 2024

Bug description

When opening an async single select combobox in dialog mode (on a mobile device), if you first make a selection, then go back and edit the text by using backspace (not removing all text), you will lose the previous selection. If you then close the modal, it will appear as if the old item is still selected, but the value of the hidden element will be blank. Re-opening the dialog again and changing the selection to anything other than the originally selected value, will result in an empty hidden value.

To Reproduce

Steps to reproduce the behavior:

  1. Go to hotwirecombobox.com
  2. Scroll down to 'An async combobox' example
  3. Make sure you have a mobile-sized viewport or are using a mobile device
  4. Make a selection in the combobox
  5. Open the combobox again, and remove some of the text by hitting backspace
  6. Close the dialog by tapping on the sliver of background at the top of the screen
  7. See that the previous selection is present in the combobox
  8. Inspect the HTML to see that the value property of the hidden element is blank

Expected behavior

If you do the same on a large viewport by editing the text with backspace, when you click outside the combobox, it will then re-select the previous item, with a correct hidden element value. I would expect the same on mobile, or at least a match between what is visually selected and what the hidden value is storing. If there is no hidden value, there should be no element selected in the combobox.

If you use the example A basic combobox instead, it will also have a correct hidden element value.

Version Numbers

HotwireCombobox info

  • Gem version: v0.3.1

Desktop info (if applicable)

  • OS: macOS 14.5
  • Browser: Chrome
  • Browser Version: 126.0.6478.115

Additional context

I'm sorry about the title of this bug report, it was difficult to find the right words without being too verbose.

@josefarias
Copy link
Owner

josefarias commented Jul 28, 2024

@ILiuz sorry about the delay here. This is due to a race condition and I really wanted to avoid creating a queue system to avoid those. But I thought about it for a long while and I don't think there's a way around it.

If you get a moment, could I get you to point your gem to this branch and verify that this is fixed there? #191

Thank you! I really appreciate the well-written report

@ILiuz
Copy link
Contributor Author

ILiuz commented Jul 28, 2024

@josefarias Thank you so much for taking a look at this!

I couldn't get this working in my actual app which uses esbuild, but on a dummy app with importmaps, everything worked as expected. The hidden input was updated!

Just tangentially related to this issue: For some reason I was not able to get any meaningful code in node_modules when I included your branch in package.json ("@josefarias/hotwire_combobox": "https://github.com/josefarias/hotwire_combobox.git#jose/queue-callbacks") when using esbuild. It was downloaded, but I got mostly just empty folders, so my app failed when trying to build the JS. This is probably just an issue on my part, and I didn't mention that I used esbuild because I was able to reproduce the issue on your docs site, which I assume uses importmaps. I will test on my actual app once you have published on NPM, or if you know what the issue might be, please let me know so I can test more.

@josefarias
Copy link
Owner

@ILiuz thank you for going out of your way to test that. Glad to know it's working. Merged now! Will be a part of the next release.

re: npm — it's set up to ignore everything but the readme and two library builds. But the building only happens on release, and the files are git-ignored. So individual branches don't have the builds and, well, testing WIP via npm is less straight forward. All that to say, this should work once the new version is released. Or you're welcome to create builds from pre-releases using yarn build.

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 a pull request may close this issue.

2 participants