fix: formdata is not updating when options populated dynamically

[caulfield]

Hi, thank you for your package, we use it in our work routine 👍

I found a bug when tried to lazy-load data to my control. We use automated way of submitting our forms and selected values were ignored. I reproduce an issue without lazy-load, even only using data option from javascript.

This is a draft pr with tests only (failed ofc). I continue my work on it tomorrow. Are you okay with that code? Maybe you have some suggestions where I can find the reason..

Thanks in advance

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[github-actions]

Playwright E2E Test Results

86 tests  +3   86 ✅ +3   2m 1s ⏱️ +20s
73 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit ee7cd4c. ± Comparison against base commit 7b8e914.

♻️ This comment has been updated with latest results.

[ghiscoding]

So you're having problem with the form data when using lazy loading? The code looks fine except that you're not actually lazy loading anything in that demo. Did you see the new Lazy Loading - Example that I added recently? I never use form data myself (I typically use .getSelects() function) and I never thought of adding tests for that. It might be helpful to add a print screen or animated gif? How did you add them dynamically? Perhaps you can try this piece of code that I suggested in this comment which might help

[caulfield]

Oh, sorry for bad explanation. I've found the issue I when tried to lazy-load data, but later found a simple scenario that represents it. This scenario is in my test example. By 'dynamically' I mean just passing data array in options during initialization.

In my thoughts, there is solution that will fix my scenario, lazy-loaded options update and, possibly, refreshOptions({ data: newData} ). But I'm afraid of some architectural tricks in lib that prevent it. I'm in progress with codebase investigations 🙃

Of course it's not a problem to check inputs right before auto-submission, detect multiple-select-vanilla'd inputs and use custom js for them. But I think it would be better to keep consistency with web platform and native browser js features.

[caulfield]

hi, @ghiscoding
it looks I finished with part related to forms. what do you think about creating a new section on demo site with forms behaviour?
btw, checking failed tests now, the list looks interesting..

[ghiscoding]

I don't think this is correct, why are you doing this? It's probably causing some of the test failures.

[caulfield]

as I understand, when I try to get value from option tag it's always a string

[ghiscoding]

yes you're right, but I wonder what are the test failures? I didn't try it locally so it's hard to know, but if you roll back this change, does that remove any of the test failures?

[caulfield]

yes, it was all green before I started.I expect some of them might be fixed in spec files, but have to check carefully

[ghiscoding]

I don't understand why you're adding a new function when there should already be code to do this, for example this code to add all rows

multiple-select-vanilla/packages/multiple-select-vanilla/src/MultipleSelectInstance.ts

Lines 580 to 583 in 85027e2

	if (this.ulElm) {
	emptyElement(this.ulElm);
	rows.forEach(itemRow => this.ulElm!.appendChild(convertItemRowToHtml(itemRow)));
	}

So why are you adding a new function? We should be able to reuse the code that already exists and creating a list from an HTML Select is nothing new, the first few examples are using that technique. So I don't understand why you're adding a new function and I would rather use and modify what already exists in the library, that is mainly to avoid making the library larger.

[caulfield]

I've added new function because I didn't see these. Thanks for sharing, I will update my changes.

[ghiscoding]

I've added new function because I didn't see these. Thanks for sharing, I will update my changes.

Yeah I even don't remember all of the code myself haha, just let me know if you need more assistance in trying to understand some part. The main reason for this function convertItemRowToHtml() that I created, was because of Virtual Scroll, I need a way to (re)create HTML elements when using virtual scroll that had good performance and that was the best I could come up with for using some kind of cache (the cache is using the row objects that is later converted to an HTML element)

[ghiscoding]

@caulfield did you need further help on this PR? I still don't fully understand the problem and the example that you modified doesn't seem to use any lazy loading which is why I don't quite understand the problem

[caulfield]

Hi. Yes. Sorry, there was a lot of other work to do. I will provide more examples to explain the issue

[Copilot]

Pull Request Overview

This PR fixes a bug where FormData values were not updating when select options were populated dynamically via the data property or lazy loading. The fix ensures that when options are created programmatically (not from HTML), the underlying select element's DOM structure is properly initialized, allowing FormData to correctly capture selected values.

Key Changes:

• Added DOM structure initialization for dynamically populated select elements
• Fixed value comparison logic to handle numeric values correctly
• Added comprehensive test coverage for FormData with both single and multiple selects

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/multiple-select-vanilla/src/utils/domUtils.ts	Added createDomStructureFromData function to build DOM elements from data objects
packages/multiple-select-vanilla/src/MultipleSelectInstance.ts	Added initHtmlRows method and logic to initialize DOM when options are from data, plus fixed value comparison
playwright/e2e/example08.spec.ts	Added tests verifying FormData updates for single and multiple selects
playwright/e2e/example07.spec.ts	Added test for lazy-loaded select FormData submission
packages/demo/src/examples/example08.ts	Added demo instances for FormData testing
packages/demo/src/examples/example08.html	Added HTML forms for FormData testing
packages/demo/src/examples/example07.ts	Added lazy-loaded select instance
packages/demo/src/examples/example07.html	Added HTML for lazy-loaded select and updated selectors

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[caulfield]

Hi, @ghiscoding , sorry for the big delay
I added example (07) how form is submitted when data were updated dynamically.
Also I didn't find how you create html elements from OptGroupRowData[] and OptionRowData[], I added it as a helper and used it.

[ghiscoding]

ok cool thanks, I will review it further this evening after work. It's nice to see that all E2E tests are passing now actually they don't all pass, could you update merge latest changes from the master/main branch? I made a few changes which one of them relates to fixes to upgraded to latest Playwright version, this might help with your PR

Will that change fix all your issues?

[ghiscoding]

note that I committed 1 copilot suggestion, but it might be worst, so it might be a good idea to rollback this commit 57aabc2

[caulfield]

thanks
yeah, it looks some e2e tests still not passed, that looks strange, anyway, I will take a look tomorrow (it's after after work in my location 😃)

Will that change fix all your issues?
if you asking about my personal project issues - yes.

about copilot commit - I would try to rollback it at first. It felt strange for me too.

[ghiscoding]

thanks yeah, it looks some e2e tests still not passed, that looks strange, anyway, I will take a look tomorrow (it's after after work in my location 😃)

Will that change fix all your issues?
if you asking about my personal project issues - yes.

about copilot commit - I would try to rollback it at first. It felt strange for me too.

the copilot is because we don't typically add wait timeout in Playwright (I often do that in Cypress but never with Playwright). I saw you have a .toString() in your PR, I wouldn't be surprised that it's causing some side effect and possible failures because of possible type differences

[ghiscoding]

you can remove this line, it's totally not needed, you previously had await page.waitForTimeout(1); but again that's not needed at all when using Playwright because we should always use async/wait in Playwright tests. It passes fine for me locally without this line

[ghiscoding]

please rollback this .toString() change, that is what make some tests fail.

[ghiscoding]

can you do a git force push to rebase the PR, because as it is now it's really hard to see what changed when the PR includes all my own recent changes.

git rebase origin/main
# fix any conflict if any
# add all git changes
git add .
git push origin HEAD --force

Also by trying it out locally, I see that you are calling your new function that you created via

if (!this.fromHtml) {
  this.initHtmlRows();
}

but I don't really like this, if I add a console log to see when it executes and then I open Example 10 (Large Select - Virtual Scroll) in the browser, it does print to the console which mean that it renders the select drop twice (1x by the previoud code initList(), and 1x by your new code initHtmlRows()) and that is not at all expected neither wanted and that is what makes Example 10 test fail.

So I'd like to know again, what are you trying to fix? Could you describe a bit more the problem you are trying to fix?

[caulfield]

So I'd like to know again, what are you trying to fix? Could you describe a bit more the problem you are trying to fix?

sure, if you comment out my rendering from this.initHtmlRows()(340 line) and check example07, you see that form doesn't see select3. All tests I added are red without my fixes. it happens because there is no options ( tag) in the dom. I don't know better solution how to fix it with virtual list without tags.

toString() was added, we discussed it before here
#419 (comment)

[ghiscoding]

sure, if you comment out my rendering from this.initHtmlRows()(340 line) and check example07, you see that form doesn't see select3. All tests I added are red without my fixes. it happens because there is no options ( tag) in the dom. I don't know better solution how to fix it with virtual list without tags.

So if I understand correctly, you added some lazy loaded data to Example07-08 and some test that are expected to work but fail without changes? So you're trying to fix an issue with the lazy load feature, however your change is affecting nearly all examples (i.e. Example10) and with your change it's loading the data twice (which is what I mentioned in my previous comment). With that in mind, let me take a look at after my working hours and there's probably an easier fix for that. I don't think that the new Util function you added is necessary but I can't be sure until I spend a bit more time on it. At least now you provided some tests that are failing without any changes, and the goal is to make these tests pass. So at least now I have a way to reproduce the bug (your new tests), and I can work with that

toString() was added, we discussed it before here
#419 (comment)

thanks for referencing previous comment, I kind of forgot since it's been a while, however I still don't want to accept this change since it does make at least 1 test fail and I think this change is probably unrelated to your issue, so let's not mix too many things and rollback this change please

[ghiscoding]

ok after trying it locally, I'm starting to understand what the issue is, so the reason that the first 2 selects from Example07 work as form data is simply because that example has <select> and <option> directly in the example.

multiple-select-vanilla/packages/demo/src/examples/example07.html

Lines 32 to 37 in 7b8e914

	<select name="select1" class="full-width">
	<option value="1">First</option>
	<option value="2">Second</option>
	<option value="3">Third</option>
	<option value="4">Fourth</option>
	</select>

If however you're providing the options through the data property (lazy or not), then the <select> is never populated. Personally I've never used form data because I always use getSelects()... So now I understand why you added the Util function, I think the idea is not that bad but I think it should be under a new flag option (maybe useFormData or whatever you want) because not everyone would want all the data to replicated in both the <select> and the <ul> list (especially for very large dataset like the Example10 for large dataset/virtual-scroll). I surely don't want that duplication myself, but I'd be ok with adding the behavior with a new flag setting. This certainly won't work and will fail badly with large dataset virtual-scroll because it will keep updating the <select> with <option> in very different order, hence why Example10 fails I think.

[ghiscoding]

this entire else section could be simplified and replaced with

createDomElement(
  'option',
  {
    value: row.value?.toString() || '',
    disabled: row.disabled || false,
    selected: !!(row.selected || false),
    className: row.classes || '',
    textContent: (row as OptionRowData).text,
  },
  parent,
);

[ghiscoding]

@caulfield did you need more help to finish this PR? I would be accepting the PR if you make the changes I mentioned in my last 2 comments. Let me know if you need more help. Cheers

[caulfield]

@ghiscoding thanks for it. I will push updates soon. I'm going to create new section in documentation for new option and restructure old sections I've updated here.