From f61ed4df4f963c0895cfe080f5d48335377613be Mon Sep 17 00:00:00 2001
From: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
Date: Tue, 1 May 2018 16:40:07 +0100
Subject: [PATCH] For checkboxes, bind to 'checked'. Fix special property
 handling in BrowserRenderer.ts. Fixes #659 and #703

* Stop the value<-->checked conversions for checkboxes. Just use native 'checked' property. Fixes #703

* Properly handle removal of 'checked' and 'value' attributes

* E2E coverage for removing 'value'
---
 .../src/Rendering/BrowserRenderer.ts          | 85 ++++++++++++-------
 .../Components/BindAttributes.cs              |  3 +-
 .../TestComponent.codegen.cs                  |  2 +-
 .../TestComponent.mappings.txt                |  2 +-
 .../Tests/BindTest.cs                         | 37 +++++++-
 .../BasicTestApp/BindCasesComponent.cshtml    |  6 ++
 6 files changed, 98 insertions(+), 37 deletions(-)

diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts
index 2a95198c5b..10f382b4f9 100644
--- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts
+++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts
@@ -84,7 +84,11 @@ export class BrowserRenderer {
           const element = getLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
           if (element instanceof HTMLElement) {
             const attributeName = renderTreeEdit.removedAttributeName(edit)!;
-            element.removeAttribute(attributeName);
+            // First try to remove any special property we use for this attribute
+            if (!this.tryApplySpecialProperty(element, attributeName, null)) {
+              // If that's not applicable, it's a regular DOM attribute so remove that
+              element.removeAttribute(attributeName);
+            }
           } else {
             throw new Error(`Cannot remove attribute from non-element child`);
           }
@@ -205,53 +209,76 @@ export class BrowserRenderer {
       return;
     }
 
-    if (attributeName === 'value') {
-      if (this.tryApplyValueProperty(toDomElement, renderTreeFrame.attributeValue(attributeFrame))) {
-        return; // If this DOM element type has special 'value' handling, don't also write it as an attribute
-      }
+    // First see if we have special handling for this attribute
+    if (!this.tryApplySpecialProperty(toDomElement, attributeName, attributeFrame)) {
+      // If not, treat it as a regular string-valued attribute
+      toDomElement.setAttribute(
+        attributeName,
+        renderTreeFrame.attributeValue(attributeFrame)!
+      );
     }
-
-    // Treat as a regular string-valued attribute
-    toDomElement.setAttribute(
-      attributeName,
-      renderTreeFrame.attributeValue(attributeFrame)!
-    );
   }
 
-  private tryApplyValueProperty(element: Element, value: string | null) {
+  private tryApplySpecialProperty(element: Element, attributeName: string, attributeFrame: RenderTreeFramePointer | null) {
+    switch (attributeName) {
+      case 'value':
+        return this.tryApplyValueProperty(element, attributeFrame);
+      case 'checked':
+        return this.tryApplyCheckedProperty(element, attributeFrame);
+      default:
+        return false;
+    }
+  }
+
+  private tryApplyValueProperty(element: Element, attributeFrame: RenderTreeFramePointer | null) {
     // Certain elements have built-in behaviour for their 'value' property
     switch (element.tagName) {
       case 'INPUT':
       case 'SELECT':
-      case 'TEXTAREA':
-        if (isCheckbox(element)) {
-          (element as HTMLInputElement).checked = value === '';
-        } else {
-          (element as any).value = value;
+      case 'TEXTAREA': {
+        const value = attributeFrame ? renderTreeFrame.attributeValue(attributeFrame) : null;
+        (element as any).value = value;
 
-          if (element.tagName === 'SELECT') {
-            // <select> is special, in that anything we write to .value will be lost if there
-            // isn't yet a matching <option>. To maintain the expected behavior no matter the
-            // element insertion/update order, preserve the desired value separately so
-            // we can recover it when inserting any matching <option>.
-            element[selectValuePropname] = value;
-          }
+        if (element.tagName === 'SELECT') {
+          // <select> is special, in that anything we write to .value will be lost if there
+          // isn't yet a matching <option>. To maintain the expected behavior no matter the
+          // element insertion/update order, preserve the desired value separately so
+          // we can recover it when inserting any matching <option>.
+          element[selectValuePropname] = value;
         }
         return true;
-      case 'OPTION':
-        element.setAttribute('value', value!);
+      }
+      case 'OPTION': {
+        const value = attributeFrame ? renderTreeFrame.attributeValue(attributeFrame) : null;
+        if (value) {
+          element.setAttribute('value', value);
+        } else {
+          element.removeAttribute('value');
+        }
         // See above for why we have this special handling for <select>/<option>
         const parentElement = element.parentElement;
         if (parentElement && (selectValuePropname in parentElement) && parentElement[selectValuePropname] === value) {
-          this.tryApplyValueProperty(parentElement, value);
+          this.tryApplyValueProperty(parentElement, attributeFrame);
           delete parentElement[selectValuePropname];
         }
         return true;
+      }
       default:
         return false;
     }
   }
 
+  private tryApplyCheckedProperty(element: Element, attributeFrame: RenderTreeFramePointer | null) {
+    // Certain elements have built-in behaviour for their 'checked' property
+    if (element.tagName === 'INPUT') {
+      const value = attributeFrame ? renderTreeFrame.attributeValue(attributeFrame) : null;
+      (element as any).checked = value !== null;
+      return true;
+    } else {
+      return false;
+    }
+  }
+
   private insertFrameRange(componentId: number, parent: LogicalElement, childIndex: number, frames: System_Array<RenderTreeFramePointer>, startIndex: number, endIndexExcl: number): number {
     const origChildIndex = childIndex;
     for (let index = startIndex; index < endIndexExcl; index++) {
@@ -281,10 +308,6 @@ function countDescendantFrames(frame: RenderTreeFramePointer): number {
   }
 }
 
-function isCheckbox(element: Element) {
-  return element.tagName === 'INPUT' && element.getAttribute('type') === 'checkbox';
-}
-
 function raiseEvent(event: Event, browserRendererId: number, componentId: number, eventHandlerId: number, eventArgs: EventForDotNet<UIEventArgs>) {
   event.preventDefault();
 
diff --git a/src/Microsoft.AspNetCore.Blazor/Components/BindAttributes.cs b/src/Microsoft.AspNetCore.Blazor/Components/BindAttributes.cs
index 9760256400..15addbcc3d 100644
--- a/src/Microsoft.AspNetCore.Blazor/Components/BindAttributes.cs
+++ b/src/Microsoft.AspNetCore.Blazor/Components/BindAttributes.cs
@@ -15,8 +15,7 @@ namespace Microsoft.AspNetCore.Blazor.Components
     // when a specific type attribute is applied.
     [BindInputElement(null, null, "value", "onchange")]
 
-    // For right now, the BrowserRenderer translates the value attribute to the checked attribute.
-    [BindInputElement("checkbox", null, "value", "onchange")]
+    [BindInputElement("checkbox", null, "checked", "onchange")]
     [BindInputElement("text", null, "value", "onchange")]
 
     [BindElement("select", null, "value", "onchange")]
diff --git a/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.codegen.cs b/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.codegen.cs
index 8ec4cc80a6..f0b1ac8d65 100644
--- a/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.codegen.cs
+++ b/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.codegen.cs
@@ -15,7 +15,7 @@ namespace Test
             base.BuildRenderTree(builder);
             builder.OpenElement(0, "input");
             builder.AddAttribute(1, "type", "checkbox");
-            builder.AddAttribute(2, "value", Microsoft.AspNetCore.Blazor.Components.BindMethods.GetValue(Enabled));
+            builder.AddAttribute(2, "checked", Microsoft.AspNetCore.Blazor.Components.BindMethods.GetValue(Enabled));
             builder.AddAttribute(3, "onchange", Microsoft.AspNetCore.Blazor.Components.BindMethods.SetValueHandler(__value => Enabled = __value, Enabled));
             builder.CloseElement();
         }
diff --git a/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.mappings.txt b/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.mappings.txt
index bac58d7ca5..b8713640dc 100644
--- a/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.mappings.txt
+++ b/test/Microsoft.AspNetCore.Blazor.Build.Test/TestFiles/RuntimeCodeGenerationTest/BuiltIn_BindToInputCheckbox_WritesAttributes/TestComponent.mappings.txt
@@ -2,7 +2,7 @@ Source Location: (86:2,12 [41] x:\dir\subdir\Test\TestComponent.cshtml)
 |
     public bool Enabled { get; set; }
 |
-Generated Location: (1037:23,12 [41] )
+Generated Location: (1039:23,12 [41] )
 |
     public bool Enabled { get; set; }
 |
diff --git a/test/Microsoft.AspNetCore.Blazor.E2ETest/Tests/BindTest.cs b/test/Microsoft.AspNetCore.Blazor.E2ETest/Tests/BindTest.cs
index be32b10252..249628d618 100644
--- a/test/Microsoft.AspNetCore.Blazor.E2ETest/Tests/BindTest.cs
+++ b/test/Microsoft.AspNetCore.Blazor.E2ETest/Tests/BindTest.cs
@@ -28,14 +28,25 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
         {
             var target = Browser.FindElement(By.Id("textbox-initially-blank"));
             var boundValue = Browser.FindElement(By.Id("textbox-initially-blank-value"));
+            var mirrorValue = Browser.FindElement(By.Id("textbox-initially-blank-mirror"));
+            var setNullButton = Browser.FindElement(By.Id("textbox-initially-blank-setnull"));
             Assert.Equal(string.Empty, target.GetAttribute("value"));
             Assert.Equal(string.Empty, boundValue.Text);
+            Assert.Equal(string.Empty, mirrorValue.GetAttribute("value"));
 
-            // Modify target; verify value is updated
+            // Modify target; verify value is updated and that textboxes linked to the same data are updated
             target.SendKeys("Changed value");
             Assert.Equal(string.Empty, boundValue.Text); // Doesn't update until change event
+            Assert.Equal(string.Empty, mirrorValue.GetAttribute("value"));
             target.SendKeys("\t");
             Assert.Equal("Changed value", boundValue.Text);
+            Assert.Equal("Changed value", mirrorValue.GetAttribute("value"));
+
+            // Remove the value altogether
+            setNullButton.Click();
+            Assert.Equal(string.Empty, target.GetAttribute("value"));
+            Assert.Equal(string.Empty, boundValue.Text);
+            Assert.Equal(string.Empty, mirrorValue.GetAttribute("value"));
         }
 
         [Fact]
@@ -43,13 +54,23 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
         {
             var target = Browser.FindElement(By.Id("textbox-initially-populated"));
             var boundValue = Browser.FindElement(By.Id("textbox-initially-populated-value"));
+            var mirrorValue = Browser.FindElement(By.Id("textbox-initially-populated-mirror"));
+            var setNullButton = Browser.FindElement(By.Id("textbox-initially-populated-setnull"));
             Assert.Equal("Hello", target.GetAttribute("value"));
             Assert.Equal("Hello", boundValue.Text);
+            Assert.Equal("Hello", mirrorValue.GetAttribute("value"));
 
-            // Modify target; verify value is updated
+            // Modify target; verify value is updated and that textboxes linked to the same data are updated
             target.Clear();
             target.SendKeys("Changed value\t");
             Assert.Equal("Changed value", boundValue.Text);
+            Assert.Equal("Changed value", mirrorValue.GetAttribute("value"));
+
+            // Remove the value altogether
+            setNullButton.Click();
+            Assert.Equal(string.Empty, target.GetAttribute("value"));
+            Assert.Equal(string.Empty, boundValue.Text);
+            Assert.Equal(string.Empty, mirrorValue.GetAttribute("value"));
         }
         
         [Fact]
@@ -86,6 +107,7 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
         {
             var target = Browser.FindElement(By.Id("checkbox-initially-unchecked"));
             var boundValue = Browser.FindElement(By.Id("checkbox-initially-unchecked-value"));
+            var invertButton = Browser.FindElement(By.Id("checkbox-initially-unchecked-invert"));
             Assert.False(target.Selected);
             Assert.Equal("False", boundValue.Text);
 
@@ -93,6 +115,11 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
             target.Click();
             Assert.True(target.Selected);
             Assert.Equal("True", boundValue.Text);
+
+            // Modify data; verify checkbox is updated
+            invertButton.Click();
+            Assert.False(target.Selected);
+            Assert.Equal("False", boundValue.Text);
         }
 
         [Fact]
@@ -100,6 +127,7 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
         {
             var target = Browser.FindElement(By.Id("checkbox-initially-checked"));
             var boundValue = Browser.FindElement(By.Id("checkbox-initially-checked-value"));
+            var invertButton = Browser.FindElement(By.Id("checkbox-initially-checked-invert"));
             Assert.True(target.Selected);
             Assert.Equal("True", boundValue.Text);
 
@@ -107,6 +135,11 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
             target.Click();
             Assert.False(target.Selected);
             Assert.Equal("False", boundValue.Text);
+
+            // Modify data; verify checkbox is updated
+            invertButton.Click();
+            Assert.True(target.Selected);
+            Assert.Equal("True", boundValue.Text);
         }
 
         [Fact]
diff --git a/test/testapps/BasicTestApp/BindCasesComponent.cshtml b/test/testapps/BasicTestApp/BindCasesComponent.cshtml
index 4172ef4b55..14cc7a69a5 100644
--- a/test/testapps/BasicTestApp/BindCasesComponent.cshtml
+++ b/test/testapps/BasicTestApp/BindCasesComponent.cshtml
@@ -4,11 +4,15 @@
     Initially blank:
     <input id="textbox-initially-blank" bind="textboxInitiallyBlankValue" />
     <span id="textbox-initially-blank-value">@textboxInitiallyBlankValue</span>
+    <input id="textbox-initially-blank-mirror" bind="textboxInitiallyBlankValue" readonly />
+    <button id="textbox-initially-blank-setnull" onclick="@(() => { textboxInitiallyBlankValue = null; })">Set null</button>
 </p>
 <p>
     Initially populated:
     <input id="textbox-initially-populated" bind="textboxInitiallyPopulatedValue" />
     <span id="textbox-initially-populated-value">@textboxInitiallyPopulatedValue</span>
+    <input id="textbox-initially-populated-mirror" bind="textboxInitiallyPopulatedValue" readonly />
+    <button id="textbox-initially-populated-setnull" onclick="@(() => { textboxInitiallyPopulatedValue = null; })">Set null</button>
 </p>
 
 <h2>Text Area</h2>
@@ -28,11 +32,13 @@
     Initially unchecked:
     <input id="checkbox-initially-unchecked" bind="checkboxInitiallyUncheckedValue" type="checkbox" />
     <span id="checkbox-initially-unchecked-value">@checkboxInitiallyUncheckedValue</span>
+    <button id="checkbox-initially-unchecked-invert" onclick="@(() => { checkboxInitiallyUncheckedValue = !checkboxInitiallyUncheckedValue; })">Invert</button>
 </p>
 <p>
     Initially checked:
     <input id="checkbox-initially-checked" bind="checkboxInitiallyCheckedValue" type="checkbox" />
     <span id="checkbox-initially-checked-value">@checkboxInitiallyCheckedValue</span>
+    <button id="checkbox-initially-checked-invert" onclick="@(() => { checkboxInitiallyCheckedValue = !checkboxInitiallyCheckedValue; })">Invert</button>
 </p>
 
 <h2>Select</h2>