From dbb4fcf334f6fc032b2b62a25fae54fa08dc00df Mon Sep 17 00:00:00 2001
From: Alexander Marks <aomarks@google.com>
Date: Thu, 15 Aug 2019 13:45:00 -0700
Subject: [PATCH] Fix broken checked property in <mwc-radio>

Previously, we didn't trigger the SelectionController (the class that
synchronizes radio button groups separated by a shadow root) when the
checked property was set, so the previously checked radio in a group was
not unchecked when a new one was checked.

This was more complicated than just triggering it from the existing
property @observer, because that gives us batched async changes (via the
UpdatingElement updated callback), so we can't reconstruct the order of
checked sets correctly. Replaces the observer with a setter that can
handle this synchronously.
---
 CHANGELOG.md                         |  2 +
 packages/radio/src/mwc-radio-base.ts | 48 ++++++++++++++++++++---
 test/unit/mwc-radio.test.js          | 57 ++++++++++++++++++++++++----
 3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 679dc0fee7..a08dbffb55 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Improve README for `<mwc-button>`
 - Improve README for `<mwc-icon-button>`
 - Split toggling icon button out into `@material/mwc-icon-button-toggle` with tag name `<mwc-icon-button-toggle>`
+- Fix bug where setting the `checked` property on an `<mwc-radio>` did not
+  result in the other radios in the group becoming unchecked.
 
 ## [0.6.0] - 2019-06-05
 - Upgrade lerna to 3.x
diff --git a/packages/radio/src/mwc-radio-base.ts b/packages/radio/src/mwc-radio-base.ts
index 06425793aa..14e0208ef7 100644
--- a/packages/radio/src/mwc-radio-base.ts
+++ b/packages/radio/src/mwc-radio-base.ts
@@ -24,11 +24,46 @@ export class RadioBase extends FormElement {
 
   @query('input') protected formElement!: HTMLInputElement;
 
-  @property({type: Boolean})
-  @observer(function(this: RadioBase, checked: boolean) {
-    this.formElement.checked = checked;
-  })
-  checked = false;
+  private _checked = false;
+
+  @property({type: Boolean, reflect: true})
+  get checked() {
+    return this._checked;
+  }
+
+  /**
+   * We define our own getter/setter for `checked` because we need to track
+   * changes to it synchronously.
+   *
+   * The order in which the `checked` property is set across radio buttons
+   * within the same group is very important. However, we can't rely on
+   * UpdatingElement's `updated` callback to observe these changes (which is
+   * also what the `@observer` decorator uses), because it batches changes to
+   * all properties.
+   *
+   * Consider:
+   *
+   *   radio1.disabled = true;
+   *   radio2.checked = true;
+   *   radio1.checked = true;
+   *
+   * In this case we'd first see all changes for radio1, and then for radio2,
+   * and we couldn't tell that radio1 was the most recently checked.
+   */
+  set checked(checked: boolean) {
+    const oldValue = this._checked;
+    if (!!checked === !!oldValue) {
+      return;
+    }
+    this._checked = checked;
+    if (this.formElement) {
+      this.formElement.checked = checked;
+    }
+    if (this._selectionController) {
+      this._selectionController.update(this);
+    }
+    this.requestUpdate('checked', oldValue);
+  }
 
   @property({type: Boolean})
   @observer(function(this: RadioBase, disabled: boolean) {
@@ -127,6 +162,9 @@ export class RadioBase extends FormElement {
 
   firstUpdated() {
     super.firstUpdated();
+    // We might not have been able to synchronize this from the checked setter
+    // earlier, if checked was set before the input was stamped.
+    this.formElement.checked = this.checked;
     if (this._selectionController) {
       this._selectionController.update(this);
     }
diff --git a/test/unit/mwc-radio.test.js b/test/unit/mwc-radio.test.js
index a8f9ee7753..ad9eed7ab1 100644
--- a/test/unit/mwc-radio.test.js
+++ b/test/unit/mwc-radio.test.js
@@ -14,22 +14,65 @@
  * limitations under the License.
  */
 
-import {assert} from 'chai';
 import {Radio} from '@material/mwc-radio';
+import {assert} from 'chai';
+import {html, render} from 'lit-html';
 
-let element;
+let container;
 
 suite('mwc-radio');
 
-beforeEach(() => {
-  element = document.createElement('mwc-radio');
-  document.body.appendChild(element);
+before(() => {
+  container = document.createElement('main');
+  document.body.appendChild(container);
 });
 
 afterEach(() => {
-  document.body.removeChild(element);
+  render(html``, container);
+});
+
+after(() => {
+  document.body.removeChild(container);
 });
 
 test('initializes as an mwc-radio', () => {
-  assert.instanceOf(element, Radio);
+  const radio = document.createElement('mwc-radio');
+  container.appendChild(radio);
+  assert.instanceOf(radio, Radio);
+});
+
+test('radio group', async () => {
+  render(
+      html`
+      <mwc-radio name="a"></mwc-group>
+      <mwc-radio name="a"></mwc-group>
+      <mwc-radio name="b"></mwc-group>
+      `,
+      container);
+
+  const [a1, a2, b1] = [...container.querySelectorAll('mwc-radio')];
+  assert.isFalse(a1.checked);
+  assert.isFalse(a2.checked);
+  assert.isFalse(b1.checked);
+
+  a1.checked = true;
+  assert.isTrue(a1.checked);
+  assert.isFalse(a2.checked);
+  assert.isFalse(b1.checked);
+
+  a2.checked = true;
+  assert.isFalse(a1.checked);
+  assert.isTrue(a2.checked);
+  assert.isFalse(b1.checked);
+
+  b1.checked = true;
+  assert.isFalse(a1.checked);
+  assert.isTrue(a2.checked);
+  assert.isTrue(b1.checked);
+
+  a2.checked = false;
+  b1.checked = false;
+  assert.isFalse(a1.checked);
+  assert.isFalse(a2.checked);
+  assert.isFalse(b1.checked);
 });