From ef69a2476b1ca15d822eb6d9397514461ecffccf Mon Sep 17 00:00:00 2001
From: Ben Scott <ben.scott@shopify.com>
Date: Mon, 17 Dec 2018 12:07:30 -0800
Subject: [PATCH] Ensure Buttons with a url prop output valid HTML

Always render a `<button disabled>` when rendering disabled Buttons
as `<a disabled>` is not valid html
---
 UNRELEASED.md                               |  2 +
 src/components/Button/Button.tsx            | 42 +++++++++++++--------
 src/components/Button/tests/Button.test.tsx | 12 +++---
 3 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/UNRELEASED.md b/UNRELEASED.md
index 9c5851de5d3..569d7804dd4 100644
--- a/UNRELEASED.md
+++ b/UNRELEASED.md
@@ -16,6 +16,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
 
 ### Bug fixes
 
+- Ensure disabled `Button` components with a `url` prop output valid HTML ([#773](https://github.com/Shopify/polaris-react/pull/773))
+
 ### Documentation
 
 - Added deprecation guidelines ([#853](https://github.com/Shopify/polaris-react/pull/853))
diff --git a/src/components/Button/Button.tsx b/src/components/Button/Button.tsx
index de1a38d4f28..0bb015dc550 100644
--- a/src/components/Button/Button.tsx
+++ b/src/components/Button/Button.tsx
@@ -152,22 +152,32 @@ function Button({
 
   const type = submit ? 'submit' : 'button';
 
-  return url ? (
-    <UnstyledLink
-      id={id}
-      url={url}
-      external={external}
-      onClick={onClick}
-      onFocus={onFocus}
-      onBlur={onBlur}
-      onMouseUp={handleMouseUpByBlurring}
-      className={className}
-      disabled={isDisabled}
-      aria-label={accessibilityLabel}
-    >
-      {content}
-    </UnstyledLink>
-  ) : (
+  if (url) {
+    return isDisabled ? (
+      // Render an `<a>` so toggling disabled/enabled state changes only the
+      // `href` attribute instead of replacing the whole element.
+      // eslint-disable-next-line jsx-a11y/anchor-is-valid
+      <a id={id} className={className} aria-label={accessibilityLabel}>
+        {content}
+      </a>
+    ) : (
+      <UnstyledLink
+        id={id}
+        url={url}
+        external={external}
+        onClick={onClick}
+        onFocus={onFocus}
+        onBlur={onBlur}
+        onMouseUp={handleMouseUpByBlurring}
+        className={className}
+        aria-label={accessibilityLabel}
+      >
+        {content}
+      </UnstyledLink>
+    );
+  }
+
+  return (
     <button
       id={id}
       type={type}
diff --git a/src/components/Button/tests/Button.test.tsx b/src/components/Button/tests/Button.test.tsx
index e6fa76a1426..17384f2f7fd 100644
--- a/src/components/Button/tests/Button.test.tsx
+++ b/src/components/Button/tests/Button.test.tsx
@@ -60,30 +60,30 @@ describe('<Button />', () => {
   });
 
   describe('disabled', () => {
-    it('sets the disabled attribute on the button', () => {
+    it('disable without a url renders <button disabled>', () => {
       const button = shallowWithAppProvider(<Button disabled />);
       expect(button.find('button').prop('disabled')).toBeTruthy();
     });
 
-    it('sets the disabled attribute on the link', () => {
+    it('disable with a url renders <a> without an href (as <a disabled> is invalid HTML)>', () => {
       const button = shallowWithAppProvider(
         <Button disabled url="http://google.com" />,
       );
-      expect(button.find(UnstyledLink).prop('disabled')).toBeTruthy();
+      expect(button.find('a').prop('href')).toBeFalsy();
     });
   });
 
   describe('loading', () => {
-    it('sets the disabled attribute on the button', () => {
+    it('loading without a url renders <button disabled>', () => {
       const button = shallowWithAppProvider(<Button loading />);
       expect(button.find('button').prop('disabled')).toBe(true);
     });
 
-    it('sets the disabled attribute on the link', () => {
+    it('loading with a url renders <a> without an href (as <a disabled> is invalid HTML)', () => {
       const button = shallowWithAppProvider(
         <Button loading url="http://google.com" />,
       );
-      expect(button.find(UnstyledLink).prop('disabled')).toBeTruthy();
+      expect(button.find('a').prop('href')).toBeFalsy();
     });
 
     it('renders a spinner into the button', () => {