Preact: [10.0.0-alpha.2] Controlled form field rendered incorrectly after user input is "undone" via state

[natevw]

I have implemented a somewhat "recalcitrant" form field, which in certain cases refuses to leave itself with the value a user set. This select group is fully "controlled" in that it uses selected/oninput rather than defaultSelected. This usually works except in one case:

If, after a user selects some other value, the value that I then render happens to be the value the group had before they selected, the user's selection is left as-is.

E.g. say I have a select box with four options: none/red/green/blue. Whenever the user selects "none" I choose "blue" instead (bear with me; the paired select box code below is the real scenario):

• If the box starts out "red" and the user selects "none", the state and the box end up "blue"
• If the box starts out "green" and the user selects "none", the state and the box end up "blue"
• But if the box starts out "blue" and the user selects "none", the state ends up "blue" but the box stays as "none"!

I suspect something might be going wrong in the diffing, i.e. when the diffing sees that the new vdom value is the same as the old vdom value and it doesn't bother to update? (There may be some irony in me complaining about this, since I celebrated what was perhaps this very behavior in the context of #1294 at least as far as unspecified props and/or mutated children went…)

Sample code

This code below has both a SingleSelect which demonstrates the issue above, as well as a DoubleSelect component to show the plausibility of why I would actually do this: when the user clears out the first of a pair of select boxes, if there is a remaining value (in the second box) it should move into the first. It works fine if the boxes have different values, but when they start out both the same, they end up both displaying "[none]" even though when the first has a value.

const SingleSelect = () => {
  const options = ['red', 'green', 'blue'];
  let [keyA, setKeyA] = useState(null);
  console.log("rendering:", keyA);

  return html`<div>
    <label>Color: </label>
    <select oninput=${evt => {
      let key = evt.target.value;
      if (key === '-') {
        setKeyA('blue');
      } else {
        setKeyA(key);
      }
    }}>
      <option value="-" selected=${!keyA}>[none]</option>
      ${options.map(key => html`<option value=${key} selected=${key === keyA}>${key}</option>`)}
    </select>
  </div>`
};

const DoubleSelect = () => {
  const options = ['red', 'green', 'blue'];
  let [keyA, setKeyA] = useState(null);
  let [keyB, setKeyB] = useState(null);
  console.log("rendering:", keyA, keyB);

  return html`<div>
    <label>Colors: </label>
    <select oninput=${evt => {
      let key = evt.target.value;
      if (key === '-') {
        setKeyA(keyB);
        setKeyB(null);
      } else {
        setKeyA(key);
      }
    }}>
      <option value="-" selected=${!keyA}>[none]</option>
      ${options.map(key => html`<option value=${key} selected=${key === keyA}>${key}</option>`)}
    </select>
    <select oninput=${evt => {
      let key = evt.target.value;
      setKeyB((key !== '-') ? key : null);
    }}>
      <option value="-" selected=${!keyB}>[none]</option>
      ${options.map(key => html`<option value=${key} selected=${key === keyB}>${key}</option>`)}
    </select>
  </div>`
};

render(html`
  <${DoubleSelect} />
  <${SingleSelect} />
`, document.body);

[marvinhagemeister]

Can you create a running version of that code in codesandbox? The issue sounds related to #1324 which is fixed in master.

[mischnic]

when the diffing sees that the new vdom value is the same as the old vdom value and it doesn't bother to update

This might already be fixed with https://github.com/developit/preact/pull/1438 (I had a very similar issue: #1410)

[natevw]

I've so far been unable to make a CodeSandbox pointing to e.g. "preact": "developit/preact#750189b" (I think due to the repo itself missing the published dist folder).

The issue I'm reporting here definitely sounds like the recent fix though. I can re-test later in the day locally at least.

[natevw]

@marvinhagemeister Here's a CodeSandbox running against 10.0.0-alpha2 showing the issue: https://codesandbox.io/s/rj0klnnn1p

I made another attempt at testing the fix after building a checkout of this repo locally, but due to the interdependencies of preact and preact/hooks it would require more fussing with the build process and/or rewriting my example and/or global npm links than it seems worth given how likely this is to be a dupe of #1410.

Feel free to close if desired, otherwise I can retest when the latest gets published as something like 10.0.0-alpha.3.

[JoviDeCroock]

This seems to work correctly when updating to alpha.3, feel free to reopen if you run into another related issue. Thanks for opening this issue

[natevw]

So I've updated both the sandbox at https://codesandbox.io/s/rj0klnnn1p and my original codebase to [email protected] and curiously this is not fixed!

I wonder if this has something to do with the somewhat special relationships here between <select>/<option>s and/or the value vs. selected properties.

[developit]

It seems like the issue here is actually the use of <option selected=..> instead of <select value=..>. Since Preact favors properties over attributes where they are defined, <select value=".."> is the preferred way of setting the value of a select.

Here's a modified version of the repro sandbox showing correct functionality after being converted to use <select value>:
https://codesandbox.io/s/jjkkr7xpjw

[marvinhagemeister]

To elaborate on that: The reason is that keeping both the options selected-attributes and the select value in sync would require a bit of work and would cost us quite a bit in terms of size. With the value attribute of select we can reuse the same code path from other form elements :tada:

[natevw]

Ah, yes. The value property of the <select> itself should
do what I need on setting, just like I ended up using it on input:

On setting, the value attribute must set the selectedness of all the option elements in the list of options to false, and then the first option element in the list of options, in tree order, whose value is equal to the given new value, if any, must have its selectedness set to true and its dirtiness set to true.

— https://www.w3.org/TR/html52/sec-forms.html#dom-htmlselectelement-value

In the case of a multi-select box, where the selectedOptions property is read-only, I wonder if this would be a problem though? In my case the above will work fine as a workaround, but I would think in general the selected property will also need to get synced for some use cases.

[marvinhagemeister]

That's a good point about a multi-select box. In react or compat we allow an array to be passed into the select's value property. We need to think about moving this functionality to core.

<select value={['A', 'C']}>
  <option value="A">A</option>
  <option value="B">B</option>
  <option value="C">C</option>
</select>

[natevw]

Still have a lingering bug in my own codebase that's at least somewhat related to this situation, albeit manifesting a bit differently than in my OP. Still haven't figured out how exactly Preact ends up getting out of sync when I set properties on the <option> elements and don't touch the <select> itself — ideally it seems Preact would naïvely cause the browser to do the right thing.

That is, directly setting selectEl.selected = true would be reliable so it's unclear why rendering <select selected=${true} /> isn't since afaict Preact generally just blindly copies vdom props to the DOM, i.e. without awareness of any of their semantics. Perhaps it's related to this bit of code: https://github.com/preactjs/preact/blob/8825bf1/src/diff/children.js#L176-L187

UPDATE: ah, yes there is also some funny business going on with properties named 'value' and 'checked' as seen in https://github.com/preactjs/preact/commit/05e5d2c0d2d92c5478eeffdbd96681c96500d29f. I haven't yet figured out why the sort of else clause for that happens elsewhere (https://github.com/preactjs/preact/blob/8825bf1/src/diff/index.js#L413-L426) when the generic setProperty gets skipped for those two special names. (And also 'selected' isn't one of the skipped-over ones so it may not be relevant here anyway?)