From 988a21e9063349eb053507ce30995fa41ea1f449 Mon Sep 17 00:00:00 2001
From: Chris Garrett <cgarrett@linkedin.com>
Date: Fri, 12 Feb 2021 10:27:17 -0800
Subject: [PATCH] [BUGFIX] Fix reactivity of dynamic attributes

The updates to use autotracking through the VM resulted in a bug in
dynamic attributes. Previously, the updating opcode for dynamic
attributes would run its update function based on tags/autotracking -
whenever any tag updated (e.g. a tracked value was changed) the
attribute would call its `update` function. The `update` function would
then check the new value against the current value of the attribute, and
update if they were different.

The bug was we introduced a value comparison in the updating opcode
itself, based on the last value of the property to set the attribute to.
This meant that if the property was updated to the same value twice in
a row, it would not call `update`, even if the attribute's value had
changed (e.g. because the user typed something).
---
 .../integration-tests/test/attributes-test.ts | 37 ++++++++++++++++++-
 .../runtime/lib/compiled/opcodes/component.ts |  2 +-
 .../runtime/lib/compiled/opcodes/dom.ts       | 34 ++++++++++-------
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/packages/@glimmer/integration-tests/test/attributes-test.ts b/packages/@glimmer/integration-tests/test/attributes-test.ts
index 20408169aa..279ae2d081 100644
--- a/packages/@glimmer/integration-tests/test/attributes-test.ts
+++ b/packages/@glimmer/integration-tests/test/attributes-test.ts
@@ -1,6 +1,7 @@
 import { normalizeProperty } from '@glimmer/runtime';
-import { assertElement, hasAttribute, jitSuite, RenderTest, test } from '..';
+import { assertElement, hasAttribute, jitSuite, RenderTest, test, tracked } from '..';
 import { Namespace, SimpleElement } from '@simple-dom/interface';
+import { castToBrowser, expect } from '@glimmer/util';
 
 export class AttributesTests extends RenderTest {
   static suiteName = 'Attributes';
@@ -239,6 +240,40 @@ export class AttributesTests extends RenderTest {
     this.assertStableNodes();
   }
 
+  @test
+  'handles successive updates to the same value'() {
+    class Model {
+      @tracked value = '';
+    }
+
+    let model = new Model();
+
+    this.render('<input value={{this.model.value}} />', { model });
+    this.assert.equal(this.readDOMAttr('value'), '');
+    this.assertStableRerender();
+
+    let inputElement = castToBrowser(
+      expect(this.element.firstChild, 'expected input to exist'),
+      'input'
+    );
+
+    inputElement.value = 'bar';
+    this.assert.equal(this.readDOMAttr('value'), 'bar');
+
+    model.value = 'foo';
+    this.rerender();
+    this.assert.equal(this.readDOMAttr('value'), 'foo');
+    this.assertStableNodes();
+
+    inputElement.value = 'bar';
+    this.assert.equal(this.readDOMAttr('value'), 'bar');
+
+    model.value = 'foo';
+    this.rerender();
+    this.assert.equal(this.readDOMAttr('value'), 'foo');
+    this.assertStableNodes();
+  }
+
   @test
   'input[checked] prop updates when set to undefined'() {
     this.registerHelper('if', (params) => {
diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts
index 48c2205afb..4c9bb5c9c5 100644
--- a/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts
+++ b/packages/@glimmer/runtime/lib/compiled/opcodes/component.ts
@@ -544,7 +544,7 @@ function setDeferredAttr(
       .elements()
       .setDynamicAttribute(name, valueForRef(value), trusting, namespace);
     if (!isConstRef(value)) {
-      vm.updateWith(new UpdateDynamicAttributeOpcode(value, attribute));
+      vm.updateWith(new UpdateDynamicAttributeOpcode(value, attribute, vm.env));
     }
   }
 }
diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
index 73f879f2fd..4b6b9fe896 100644
--- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
+++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
@@ -1,4 +1,4 @@
-import { Reference, valueForRef, isConstRef } from '@glimmer/reference';
+import { Reference, valueForRef, isConstRef, createComputeRef } from '@glimmer/reference';
 import { Revision, Tag, valueForTag, validateTag, consumeTag } from '@glimmer/validator';
 import {
   check,
@@ -17,6 +17,7 @@ import {
   Owner,
   CurriedType,
   ModifierDefinitionState,
+  Environment,
 } from '@glimmer/interfaces';
 import { $t0 } from '@glimmer/vm';
 import { APPEND_OPCODES, UpdatingOpcode } from '../../opcodes';
@@ -237,27 +238,34 @@ APPEND_OPCODES.add(Op.DynamicAttr, (vm, { op1: _name, op2: _trusting, op3: _name
   let attribute = vm.elements().setDynamicAttribute(name, value, trusting, namespace);
 
   if (!isConstRef(reference)) {
-    vm.updateWith(new UpdateDynamicAttributeOpcode(reference, attribute));
+    vm.updateWith(new UpdateDynamicAttributeOpcode(reference, attribute, vm.env));
   }
 });
 
 export class UpdateDynamicAttributeOpcode extends UpdatingOpcode {
   public type = 'patch-element';
 
-  public lastValue: unknown;
+  private updateRef: Reference;
 
-  constructor(private reference: Reference<unknown>, private attribute: DynamicAttribute) {
+  constructor(reference: Reference<unknown>, attribute: DynamicAttribute, env: Environment) {
     super();
-    this.lastValue = valueForRef(reference);
-  }
 
-  evaluate(vm: UpdatingVM) {
-    let { attribute, reference, lastValue } = this;
-    let currentValue = valueForRef(reference);
+    let initialized = false;
 
-    if (currentValue !== lastValue) {
-      attribute.update(currentValue, vm.env);
-      this.lastValue = currentValue;
-    }
+    this.updateRef = createComputeRef(() => {
+      let value = valueForRef(reference);
+
+      if (initialized === true) {
+        attribute.update(value, env);
+      } else {
+        initialized = true;
+      }
+    });
+
+    valueForRef(this.updateRef);
+  }
+
+  evaluate() {
+    valueForRef(this.updateRef);
   }
 }