actually fix #684 (#696)

* actually fix #684

Because we handle events asynchronously, we must leave the value
uncontrolled in order to allow all changed committed by the user to be
recorded in the order they occur. If we don't, the user may commit
multiple changes before we render resulting in prior changes being
overwritten by subsequent ones. Instead of controlling the value, we set
it in an effect. This feels a bit hacky, but I can't think of a better
way to work around this problem.

* update changelog

Author: rmorshea

docs/source/_custom_js/package-lock.json

@@ -16,15 +16,17 @@ scheme for the project adheres to `Semantic Versioning <https://semver.org/>`__.
 Unreleased
 ----------
 
-Nothing yet...
+Fixed:
+
+- Revert :pull:`694` and by making ``value`` uncontrolled client-side - :issue:`684`
 
 
 0.37.1-a1
 ---------
 
 Fixed:
 
-- ``onChange`` event for inputs missing key strokes :issue:`684`
+- ``onChange`` event for inputs missing key strokes - :issue:`684`
 
 
 0.37.0

docs/source/about/changelog.rst

@@ -38,6 +38,8 @@ export function Element({ model }) {
     }
   } else if (model.tagName == "script") {
     return html`<${ScriptElement} model=${model} />`;
+  } else if (["input", "select", "textarea"].includes(model.tagName)) {
+    return html`<${UserInputElement} model=${model} />`;
   } else if (model.importSource) {
     return html`<${ImportedElement} model=${model} />`;
   } else {
@@ -68,6 +70,45 @@ function StandardElement({ model }) {
   );
 }
 
+// Element with a value attribute controlled by user input
+function UserInputElement({ model }) {
+  const ref = React.useRef();
+  const layoutContext = React.useContext(LayoutContext);
+
+  const props = createElementAttributes(model, layoutContext.sendEvent);
+
+  // Because we handle events asynchronously, we must leave the value uncontrolled in
+  // order to allow all changes committed by the user to be recorded in the order they
+  // occur. If we don't the user may commit multiple changes before we render next
+  // causing the content of prior changes to be overwritten by subsequent changes.
+  const value = props.value;
+  delete props.value;
+
+  // Instead of controlling the value, we set it in an effect.
+  React.useEffect(() => {
+    if (value !== undefined) {
+      ref.current.value = value;
+    }
+  }, [ref.current, value]);
+
+  // Use createElement here to avoid warning about variable numbers of children not
+  // having keys. Warning about this must now be the responsibility of the server
+  // providing the models instead of the client rendering them.
+  return React.createElement(
+    model.tagName,
+    {
+      ...props,
+      ref: (target) => {
+        ref.current = target;
+      },
+    },
+    ...createElementChildren(
+      model,
+      (model) => html`<${Element} key=${model.key} model=${model} />`
+    )
+  );
+}
+
 function ScriptElement({ model }) {
   const ref = React.useRef();
   React.useEffect(() => {

src/client/packages/idom-client-react/src/components.js

@@ -39,8 +39,6 @@ function createEventHandler(eventName, sendEvent, eventSpec) {
       if (typeof value === "object" && value.nativeEvent) {
         if (eventSpec["preventDefault"]) {
           value.preventDefault();
-        } else if (eventName === "onChange") {
-          value.nativeEvent.target.value = value.target.value;
         }
         if (eventSpec["stopPropagation"]) {
           value.stopPropagation();