From bacc1144627f73f15650fab1df9c14277b2ea5b8 Mon Sep 17 00:00:00 2001
From: Kris <kris.aubuchon@discourse.org>
Date: Mon, 28 Oct 2019 16:04:29 -0400
Subject: [PATCH] UX: Convert alert & modal close to buttons for improved
 accessibility

---
 .../templates/components/d-modal.hbs          |  6 +-
 .../templates/components/discourse-banner.hbs |  4 +-
 .../notification-consent-banner.hbs           |  4 +-
 .../components/pwa-install-banner.hbs         |  4 +-
 .../templates/components/share-popup.hbs      |  5 +-
 app/assets/stylesheets/common/base/alert.scss | 15 +----
 .../common/components/buttons.scss            | 67 ++++++++++---------
 .../stylesheets/common/foundation/mixins.scss |  4 +-
 8 files changed, 46 insertions(+), 63 deletions(-)

diff --git a/app/assets/javascripts/discourse/templates/components/d-modal.hbs b/app/assets/javascripts/discourse/templates/components/d-modal.hbs
index 4e7cd0dca26..6c6106bbad8 100644
--- a/app/assets/javascripts/discourse/templates/components/d-modal.hbs
+++ b/app/assets/javascripts/discourse/templates/components/d-modal.hbs
@@ -3,11 +3,7 @@
     <div class="modal-inner-container">
       <div class="modal-header">
         {{#if dismissable}}
-          <div class='modal-close'>
-            <a class="close" href {{action closeModal}}>
-              {{d-icon "times"}}
-            </a>
-          </div>
+          {{d-button icon="times" action=(route-action "closeModal") class="btn btn-flat modal-close close" }}
         {{/if}}
 
         {{#if panels}}
diff --git a/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs b/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs
index d7e0c5e1d43..ad5cccf6a9d 100644
--- a/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs
+++ b/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs
@@ -1,9 +1,7 @@
 {{#if visible}}
   <div class="row">
     <div id="banner" class={{overlay}}>
-      <div class="close" {{action "dismiss"}}>
-        {{d-icon "times" title="banner.close"}}
-      </div>
+      {{d-button icon="times" action="dismiss" class="btn btn-flat close" title="banner.close"}}
       <div id="banner-content">
         {{{banner.html}}}
         {{#if currentUser.staff}}
diff --git a/app/assets/javascripts/discourse/templates/components/notification-consent-banner.hbs b/app/assets/javascripts/discourse/templates/components/notification-consent-banner.hbs
index e93d62a3ba2..e249c7f98fd 100644
--- a/app/assets/javascripts/discourse/templates/components/notification-consent-banner.hbs
+++ b/app/assets/javascripts/discourse/templates/components/notification-consent-banner.hbs
@@ -1,9 +1,7 @@
 {{#if showNotificationPromptBanner}}
   <div class="row">
     <div class="consent_banner alert alert-info">
-      <div class="close" {{action "dismiss"}}>
-        {{d-icon 'times'}}
-      </div>
+      {{d-button icon="times" action="dismiss" class="btn btn-flat close" title="banner.close"}}
       {{i18n 'user.desktop_notifications.consent_prompt'}} <a {{action "turnon"}}>{{i18n 'user.desktop_notifications.enable'}}</a>.
     </div>
   </div>
diff --git a/app/assets/javascripts/discourse/templates/components/pwa-install-banner.hbs b/app/assets/javascripts/discourse/templates/components/pwa-install-banner.hbs
index d0391747eac..d394724ab43 100644
--- a/app/assets/javascripts/discourse/templates/components/pwa-install-banner.hbs
+++ b/app/assets/javascripts/discourse/templates/components/pwa-install-banner.hbs
@@ -1,9 +1,7 @@
 {{#if showPWAInstallBanner}}
   <div class="row">
     <div class="pwa-install-banner alert alert-info">
-      <div class="close" {{action "dismiss"}}>
-        {{d-icon 'times'}}
-      </div>
+      {{d-button icon="times" action="dismiss" class="btn btn-flat close" title="banner.close"}}
       <span>
         {{discourse-linked-text action=(action "turnOn") translatedText=(i18n "pwa.install_banner" title=siteSettings.title)}}
       </span>
diff --git a/app/assets/javascripts/discourse/templates/components/share-popup.hbs b/app/assets/javascripts/discourse/templates/components/share-popup.hbs
index 55f287720b9..6f0428b1aab 100644
--- a/app/assets/javascripts/discourse/templates/components/share-popup.hbs
+++ b/app/assets/javascripts/discourse/templates/components/share-popup.hbs
@@ -35,8 +35,7 @@
       </div>
     {{/if}}
 
-    <a href {{action "close"}} class="close-share" aria-label={{i18n "share.close"}} title={{i18n "share.close"}}>
-      {{d-icon "times"}}
-    </a>
+
+    {{d-button action="close" class="btn btn-flat close" icon="times" aria-label="share.close" title="share.close"}}
   </div>
 </div>
diff --git a/app/assets/stylesheets/common/base/alert.scss b/app/assets/stylesheets/common/base/alert.scss
index 8d05c6a1585..f605d993cff 100644
--- a/app/assets/stylesheets/common/base/alert.scss
+++ b/app/assets/stylesheets/common/base/alert.scss
@@ -8,21 +8,12 @@
     position: absolute;
     top: 8px;
     right: 8px;
-    line-height: $line-height-small;
     font-size: $font-up-3;
-    color: $primary-low-mid;
-    @include hover {
-      color: $primary-medium;
-      cursor: pointer;
+    .d-icon {
+      color: $primary-low-mid;
     }
   }
-  button.close {
-    padding: 0;
-    cursor: pointer;
-    background: transparent;
-    border: 0;
-    -webkit-appearance: none;
-  }
+
   &.alert-success {
     background-color: $success-low;
     color: $primary;
diff --git a/app/assets/stylesheets/common/components/buttons.scss b/app/assets/stylesheets/common/components/buttons.scss
index de22978dadf..018fc94bdd4 100644
--- a/app/assets/stylesheets/common/components/buttons.scss
+++ b/app/assets/stylesheets/common/components/buttons.scss
@@ -59,15 +59,11 @@
     color: $primary;
     min-height: unset; // ovverides button defaults
   }
-  .discourse-no-touch & {
-    // only allow hover on no-touch devices
-    &:hover,
-    &.btn-hover {
-      background: $primary-medium;
-      color: $secondary;
-      .d-icon {
-        color: $secondary-very-high;
-      }
+  @include hover {
+    background: $primary-medium;
+    color: $secondary;
+    .d-icon {
+      color: $secondary-very-high;
     }
   }
   &[disabled],
@@ -104,17 +100,13 @@
   &[href] {
     color: $secondary;
   }
-  .discourse-no-touch & {
-    // only allow hover on no-touch devices
-    &:hover,
-    &.btn-hover {
-      background: dark-light-choose(
-        scale-color($tertiary, $lightness: -20%),
-        scale-color($tertiary, $lightness: 20%)
-      );
-      .d-icon {
-        color: currentColor;
-      }
+  @include hover {
+    background: dark-light-choose(
+      scale-color($tertiary, $lightness: -20%),
+      scale-color($tertiary, $lightness: 20%)
+    );
+    .d-icon {
+      color: currentColor;
     }
   }
   &:active,
@@ -146,17 +138,13 @@
   &[href] {
     color: $secondary;
   }
-  .discourse-no-touch & {
-    // only allow hover on no-touch devices
-    &:hover,
-    &.btn-hover {
-      background: dark-light-choose(
-        scale-color($danger, $lightness: -20%),
-        scale-color($danger, $lightness: 20%)
-      );
-      .d-icon {
-        color: $danger-low;
-      }
+  @include hover {
+    background: dark-light-choose(
+      scale-color($danger, $lightness: -20%),
+      scale-color($danger, $lightness: 20%)
+    );
+    .d-icon {
+      color: $danger-low;
     }
   }
   &:active,
@@ -266,8 +254,21 @@
   .d-icon {
     color: $primary-low-mid;
   }
-  .discourse-no-touch & {
-    &:hover {
+  @include hover {
+    .d-icon {
+      color: $primary;
+    }
+  }
+  &.close {
+    padding: 0;
+    min-height: unset;
+    background: transparent;
+    font-size: $font-up-3;
+    .d-icon {
+      color: $primary-high;
+    }
+    @include hover {
+      background: transparent;
       .d-icon {
         color: $primary;
       }
diff --git a/app/assets/stylesheets/common/foundation/mixins.scss b/app/assets/stylesheets/common/foundation/mixins.scss
index 8a83dd6c782..99f76345478 100644
--- a/app/assets/stylesheets/common/foundation/mixins.scss
+++ b/app/assets/stylesheets/common/foundation/mixins.scss
@@ -47,9 +47,11 @@ $breakpoints: (
 // Visibility
 // --------------------------------------------------
 
+// Only shows hover on non-touch devices
 @mixin hover {
   .discourse-no-touch & {
-    &:hover {
+    &:hover,
+    &.btn-hover {
       @content;
     }
   }