Skip to content

fix(select): fix prop not working properly on hidden select input#538

Open
royeden wants to merge 1 commit intokobaltedev:mainfrom
royeden:main
Open

fix(select): fix prop not working properly on hidden select input#538
royeden wants to merge 1 commit intokobaltedev:mainfrom
royeden:main

Conversation

@royeden
Copy link
Contributor

@royeden royeden commented Dec 23, 2024

Hello!

I've noticed that <Select /> components don't honor the required attribute properly, so this PR assigns a value attribute (same one assigned to the select element) to the input inside of <HiddenSelect />.

I wanted to add a test for this issue but I ran into the pointer-events issue that makes you skip all the current Select tests.

@netlify
Copy link

netlify bot commented Dec 23, 2024

👷 Deploy request for kobalte pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 4e0c76a

@jamesarosen
Copy link

jamesarosen commented Mar 7, 2026

This also happens with the Combobox HiddenSelect.

For anyone who wants to fix this in user-land while we wait, this is working for me:

function MySelect(props) {
	/**
	 * If `props.required`, <HiddenSelect> renders an `<input required>`, but it
	 * doesn't update its value.
	 * @see https://github.com/kobaltedev/kobalte/pull/538
	 */
	const syncHiddenInputOnChange = (e: HTMLElementEventMap['change']) => {
		const select = e.currentTarget as HTMLSelectElement | undefined
		if (!select) return
		const input = select.parentElement?.querySelector('input')
		if (!input) return
		input.value = select.value
	}
	let hiddenSelect: HTMLSelectElement | undefined
	onMount(() => {
		hiddenSelect?.addEventListener('change', syncHiddenInputOnChange)
		onCleanup(() => {
			hiddenSelect?.removeEventListener('change', syncHiddenInputOnChange)
		})
	})

	// ...

	return (<Root {...props}>
		<HiddenSelect ref={hiddenSelect} />
		...
	</Root>)
}

jamesarosen added a commit to jamesarosen/PickMyFruit that referenced this pull request Mar 7, 2026
## Summary

- Adopts Kobalte UI (`@kobalte/core`) for form field components across
  the entire app, including a workaround for `<Select required>` causing
  inconsistent state.
	See kobaltedev/kobalte#538
- Establish named JSX props as slot pattern, including a workaround for
  JSX props inside a `<Show>`.
  See solidjs/solid#1977
- Migrated `ListingForm`, `InquiryForm`, and `login` to the new
  form field components
- Mitigate open redirect in login form
- E2E selectors updated from `input#email` (Kobalte generates its own
  IDs) to `page.getByLabel(/email/i)`

## Bug Fixes

| Severity | Fix |
|----------|-----|
| CRITICAL | Missing `name="address"` on ListingForm — field was silently dropped from FormData |
| CRITICAL | Kobalte `Select` missing `<HiddenSelect />` — fruit type never reached FormData |
| HIGH | `validationState` computed outside reactive graph — error styling never applied |
| HIGH | Slot props inside `<Show>` broken (solidjs/solid#1977) — fixed with reactive accessor pattern |
| HIGH | `FieldErrors.toArray()` called outside reactive context — errors never updated |
| HIGH | InquiryForm auto-submit re-fired on remount — clear `inquiry_complete` URL param via History API |
| HIGH | InquiryForm auto-submit didn't validate stored `listingId` matches current listing |
| HIGH | `returnTo` open-redirect — `/\evil.com` bypassed `startsWith('//')` check; now uses `new URL()` origin check |
| MEDIUM | `sessionStorage.setItem` unguarded `QuotaExceededError` |
| MEDIUM | `Show when={errors}` truthy for `[]` — empty error container rendered when no errors |
| MEDIUM | `Sentry.captureException` called with string instead of `Error` (no stack trace) |
| MEDIUM | `itemComponent` now required in `SelectFieldProps` — omitting it caused silent empty listbox |

## Test Plan

- [x] Create a listing: fill all fields, submit — fruit type and address appear in confirmation
- [x] Create a listing: submit without fruit type — validation error appears
- [x] Create a listing: submit without address — validation error appears
- [x] Login: invalid `returnTo` like `//evil.com` is rejected
- [x] Contact owner: magic link flow, then auto-submit on return
- [x] Unit tests: `pnpm --filter @pickmyfruit/www test:run`
- [x] Typecheck: `pnpm --filter @pickmyfruit/www typecheck`
- [x] E2E: `pnpm --filter @pickmyfruit/www test:e2e`

## Review Notes

- Named JSX props confirmed as the correct SolidJS slot pattern (no
  children classification; SSR-safe)
- `<HiddenSelect />` is required for native FormData participation —
  Kobalte doesn't auto-render it
- Kobalte auto-generates input IDs; all test selectors updated to use
  label/name/role
- InquiryForm auto-submit test coverage deferred

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jamesarosen added a commit to jamesarosen/PickMyFruit that referenced this pull request Mar 7, 2026
## Summary

- Adopts Kobalte UI (`@kobalte/core`) for form field components across
  the entire app, including a workaround for `<Select required>` causing
  inconsistent state.
	See kobaltedev/kobalte#538
- Establish named JSX props as slot pattern, including a workaround for
  JSX props inside a `<Show>`.
  See solidjs/solid#1977
- Migrated `ListingForm`, `InquiryForm`, and `login` to the new
  form field components
- Mitigate open redirect in login form
- E2E selectors updated from `input#email` (Kobalte generates its own
  IDs) to `page.getByLabel(/email/i)`

## Bug Fixes

| Severity | Fix |
|----------|-----|
| CRITICAL | Missing `name="address"` on ListingForm — field was silently dropped from FormData |
| CRITICAL | Kobalte `Select` missing `<HiddenSelect />` — fruit type never reached FormData |
| HIGH | `validationState` computed outside reactive graph — error styling never applied |
| HIGH | Slot props inside `<Show>` broken (solidjs/solid#1977) — fixed with reactive accessor pattern |
| HIGH | `FieldErrors.toArray()` called outside reactive context — errors never updated |
| HIGH | InquiryForm auto-submit re-fired on remount — clear `inquiry_complete` URL param via History API |
| HIGH | InquiryForm auto-submit didn't validate stored `listingId` matches current listing |
| HIGH | `returnTo` open-redirect — `/\evil.com` bypassed `startsWith('//')` check; now uses `new URL()` origin check |
| MEDIUM | `sessionStorage.setItem` unguarded `QuotaExceededError` |
| MEDIUM | `Show when={errors}` truthy for `[]` — empty error container rendered when no errors |
| MEDIUM | `Sentry.captureException` called with string instead of `Error` (no stack trace) |
| MEDIUM | `itemComponent` now required in `SelectFieldProps` — omitting it caused silent empty listbox |

## Test Plan

- [x] Create a listing: fill all fields, submit — fruit type and address appear in confirmation
- [x] Create a listing: submit without fruit type — validation error appears
- [x] Create a listing: submit without address — validation error appears
- [x] Login: invalid `returnTo` like `//evil.com` is rejected
- [x] Contact owner: magic link flow, then auto-submit on return
- [x] Unit tests: `pnpm --filter @pickmyfruit/www test:run`
- [x] Typecheck: `pnpm --filter @pickmyfruit/www typecheck`
- [x] E2E: `pnpm --filter @pickmyfruit/www test:e2e`

## Review Notes

- Named JSX props confirmed as the correct SolidJS slot pattern (no
  children classification; SSR-safe)
- `<HiddenSelect />` is required for native FormData participation —
  Kobalte doesn't auto-render it
- Kobalte auto-generates input IDs; all test selectors updated to use
  label/name/role
- InquiryForm auto-submit test coverage deferred to #146

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jamesarosen added a commit to jamesarosen/PickMyFruit that referenced this pull request Mar 7, 2026
## Summary

- Adopts Kobalte UI (`@kobalte/core`) for form field components across
  the entire app, including a workaround for `<Select required>` causing
  inconsistent state.
  See kobaltedev/kobalte#538
- Establish named JSX props as slot pattern, including a workaround for
  JSX props inside a `<Show>`.
  See solidjs/solid#1977
- Migrated `ListingForm`, `InquiryForm`, and `login` to the new
  form field components
- Mitigate open redirect in login form
- E2E selectors updated from `input#email` (Kobalte generates its own
  IDs) to `page.getByLabel(/email/i)`

## Bug Fixes

| Severity | Fix |
|----------|-----|
| CRITICAL | Missing `name="address"` on ListingForm — field was silently dropped from FormData |
| CRITICAL | Kobalte `Select` missing `<HiddenSelect />` — fruit type never reached FormData |
| HIGH | `validationState` computed outside reactive graph — error styling never applied |
| HIGH | Slot props inside `<Show>` broken (solidjs/solid#1977) — fixed with reactive accessor pattern |
| HIGH | `FieldErrors.toArray()` called outside reactive context — errors never updated |
| HIGH | InquiryForm auto-submit re-fired on remount — clear `inquiry_complete` URL param via History API |
| HIGH | InquiryForm auto-submit didn't validate stored `listingId` matches current listing |
| HIGH | `returnTo` open-redirect — `/\evil.com` bypassed `startsWith('//')` check; now uses `new URL()` origin check |
| MEDIUM | `sessionStorage.setItem` unguarded `QuotaExceededError` |
| MEDIUM | `Show when={errors}` truthy for `[]` — empty error container rendered when no errors |
| MEDIUM | `Sentry.captureException` called with string instead of `Error` (no stack trace) |
| MEDIUM | `itemComponent` now required in `SelectFieldProps` — omitting it caused silent empty listbox |

## Test Plan

- [x] Create a listing: fill all fields, submit — fruit type and address appear in confirmation
- [x] Create a listing: submit without fruit type — validation error appears
- [x] Create a listing: submit without address — validation error appears
- [x] Login: invalid `returnTo` like `//evil.com` is rejected
- [x] Contact owner: magic link flow, then auto-submit on return
- [x] Unit tests: `pnpm --filter @pickmyfruit/www test:run`
- [x] Typecheck: `pnpm --filter @pickmyfruit/www typecheck`
- [x] E2E: `pnpm --filter @pickmyfruit/www test:e2e`

## Review Notes

- Named JSX props confirmed as the correct SolidJS slot pattern (no
  children classification; SSR-safe)
- `<HiddenSelect />` is required for native FormData participation —
  Kobalte doesn't auto-render it
- Kobalte auto-generates input IDs; all test selectors updated to use
  label/name/role
- InquiryForm auto-submit test coverage deferred to #146

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@jamesarosen
Copy link

Actually, that doesn't quite work because it doesn't sync on mount.

I tried extracting value and passing it to both <Root> and <HiddenSelect>, but there's a type mismatch. In Root, value is Option | Option[] | null | undefined, whereas in HiddenSelect it's string | number | string[] | undefined.

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.

2 participants