Skip to content

Commit 15a2552

Browse files
avanelsasclaude
andauthored
fix(x-select): make .value reflect the current selection (#263)
x-select's `.value` returned the value ATTRIBUTE, which a user selection never updates (only the inner <select> + the dispatched select-change event changed). So consumers that read `el.value` — x-form's collect-values, the demo's status filter — saw a stale value: the filter looked dead, and forms silently submitted the pre-selection value. Every other BareDOM form control makes `.value`/`.checked` reflect the current value (the contract x-form relies on); x-select was the lone exception. Override its `value` getter to read the live inner <select>, with one disambiguation: a non-empty value set BEFORE its <option> exists (async-loaded options) is surfaced as pending via the attribute until the option arrives. This preserves both existing tests (value-attr-not-auto-set-on-change, string-property-value) and adds two regression tests (selection reflection; set-before-options). clj-kondo clean; full karma 5190. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ac00789 commit 15a2552

2 files changed

Lines changed: 84 additions & 1 deletion

File tree

src/baredom/components/x_select/x_select.cljs

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,8 +398,50 @@
398398
;; Element class and registration
399399
;; ---------------------------------------------------------------------------
400400

401+
(defn- option-value-present?
402+
"True when the inner <select> currently has an <option> with this exact value."
403+
[^js select-el value]
404+
(let [opts (.-options select-el)
405+
n (.-length opts)]
406+
(loop [i 0]
407+
(cond
408+
(>= i n) false
409+
(= value (.-value (aget opts i))) true
410+
:else (recur (inc i))))))
411+
412+
;; `value` must reflect the CURRENT selection — that is the native <select> contract and
413+
;; what every other BareDOM form control honours (x-form's collect-values and direct
414+
;; `el.value` readers depend on it). The reflecting accessor install-properties! would give
415+
;; reads the `value` ATTRIBUTE, which a user selection never updates, so override `value`
416+
;; with a getter that reads the live inner <select>. The one wrinkle: a non-empty value set
417+
;; BEFORE its <option> exists (async-loaded options) can't be held by the inner <select> —
418+
;; surface that as pending via the attribute until the option arrives and apply-model!
419+
;; selects it. (`value-attr-not-auto-set-on-change-test` still holds: a user change updates
420+
;; only the inner <select>, never the attribute.)
421+
(defn- define-value-prop! [^js proto]
422+
(.defineProperty
423+
js/Object proto "value"
424+
#js {:configurable true
425+
:enumerable true
426+
:get (fn xs-get-value []
427+
(this-as ^js this
428+
(if-let [refs (du/getv this k-refs)]
429+
(let [^js select-el (gobj/get refs part-select)
430+
attr (or (du/get-attr this model/attr-value) "")]
431+
(if (and (not= attr "") (not (option-value-present? select-el attr)))
432+
attr ; pending: option not loaded yet
433+
(.-value select-el))) ; live selection (incl. a deliberate empty pick)
434+
(or (du/get-attr this model/attr-value) ""))))
435+
:set (fn xs-set-value [v]
436+
;; Set the attribute (the controlled/pending value); apply-model! selects the
437+
;; matching <option> on the inner select. Same as the reflecting setter it replaces.
438+
(this-as ^js this
439+
(du/set-attr! this model/attr-value
440+
(if (and (some? v) (not= v js/undefined)) (str v) ""))))}))
441+
401442
(defn- install-property-accessors! [^js proto]
402-
(du/install-properties! proto model/property-api))
443+
(du/install-properties! proto model/property-api) ; disabled / required / name + (reflecting) value
444+
(define-value-prop! proto)) ; override `value`: getter reads the live inner <select>
403445

404446
(defn init! []
405447
(component/register! model/tag-name

test/baredom/components/x_select/x_select_test.cljs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,47 @@
324324
0))
325325
0))))
326326

327+
;; Conforming behaviour: `el.value` reflects the user's CURRENT selection (native <select>
328+
;; contract) — what x-form's collect-values and the demo's status filter read. Previously
329+
;; el.value returned the stale `value` attribute and never tracked a user selection.
330+
(deftest value-reflects-user-selection-test
331+
(async done
332+
(let [el (append! (make-el))
333+
opt-a (.createElement js/document "option")
334+
opt-b (.createElement js/document "option")]
335+
(.setAttribute opt-a "value" "alpha") (set! (.-textContent opt-a) "Alpha")
336+
(.setAttribute opt-b "value" "beta") (set! (.-textContent opt-b) "Beta")
337+
(.appendChild el opt-a)
338+
(.appendChild el opt-b)
339+
(js/setTimeout
340+
(fn []
341+
(let [^js sel (shadow-part el "[part=select]")]
342+
(set! (.-value sel) "beta")
343+
(.dispatchEvent sel (js/Event. "change" #js {:bubbles true})))
344+
(js/setTimeout
345+
(fn []
346+
(is (= "beta" (.-value el))
347+
"el.value reflects the user's selection (not the stale attribute)")
348+
(done))
349+
0))
350+
0))))
351+
352+
;; A value set BEFORE its <option> exists (async-loaded options) reads back as pending,
353+
;; then resolves to the live selection once the option arrives.
354+
(deftest value-set-before-options-is-pending-then-resolves-test
355+
(async done
356+
(let [el (append! (make-el))]
357+
(set! (.-value el) "beta")
358+
(is (= "beta" (.-value el)) "pending value reads back before its option exists")
359+
(let [opt-b (.createElement js/document "option")]
360+
(.setAttribute opt-b "value" "beta") (set! (.-textContent opt-b) "Beta")
361+
(.appendChild el opt-b))
362+
(js/setTimeout
363+
(fn []
364+
(is (= "beta" (.-value el)) "resolves to the now-present option's value")
365+
(done))
366+
0))))
367+
327368
(deftest select-change-event-bubbles-and-composed-test
328369
(async done
329370
(let [el (append! (make-el))

0 commit comments

Comments
 (0)