From 496f910f03dfbc0a8021561fa7795a68894ef0e4 Mon Sep 17 00:00:00 2001
From: Osama Sayegh <asooomaasoooma90@gmail.com>
Date: Tue, 20 Sep 2022 19:31:56 +0300
Subject: [PATCH] DEV: Various A11Y improvements for the new user menu (#18288)

This commit includes various accessibility improvements for the new user menu:

* Add `title` attributes to the user menu tabs
* Properly label lists (by adding `aria-labelledby` to `<ul>` elements) for screen readers
* Change the user menu structure so that the tabs come before the content panel in the DOM, but use CSS to reverse them visually.
  Normally, changing the order of elements via CSS is bad for accessibility, but I believe this is one of the rare scenarios where it [makes sense](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Ordering_Flex_Items#use_cases_for_order). Prior to this change, if you want to reach the first notification item after you select a tab using the keyboard, you have to hit <kbd>ctrl</kbd>+<kbd>tab</kbd> because the notifications list is before the tabs list. However, with this change, <kbd>tab</kbd> will move you to the first item in the list after you select a tab using your keyboard.
* Aria-hide the unread notifications badge/count on the tabs because the `title` attribute on the tab indicates the unread count.
* Add some tests.
---
 .../app/components/user-menu/items-list.hbs   |   2 +-
 .../app/components/user-menu/menu-tab.hbs     |   4 +
 .../app/components/user-menu/menu.hbs         |  26 ++--
 .../app/components/user-menu/menu.js          |   7 +-
 .../user-menu/profile-tab-content.hbs         |   2 +-
 .../app/lib/user-menu/notification-item.js    |   5 +-
 .../discourse/app/lib/user-menu/tab.js        |  22 ++++
 .../tests/acceptance/user-menu-test.js        | 115 +++++++++++++++++-
 .../components/user-menu/menu-test.js         |  10 --
 .../stylesheets/common/base/menu-panel.scss   |   2 +-
 config/locales/client.en.yml                  |  37 +++++-
 11 files changed, 196 insertions(+), 36 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/user-menu/items-list.hbs b/app/assets/javascripts/discourse/app/components/user-menu/items-list.hbs
index e54cd281f62..af8548d029a 100644
--- a/app/assets/javascripts/discourse/app/components/user-menu/items-list.hbs
+++ b/app/assets/javascripts/discourse/app/components/user-menu/items-list.hbs
@@ -3,7 +3,7 @@
     <div class="spinner"></div>
   </div>
 {{else if this.items.length}}
-  <ul>
+  <ul aria-labelledby={{@ariaLabelledby}}>
     {{#each this.items as |item|}}
       <UserMenu::MenuItem @item={{item}} @closeUserMenu={{@closeUserMenu}}/>
     {{/each}}
diff --git a/app/assets/javascripts/discourse/app/components/user-menu/menu-tab.hbs b/app/assets/javascripts/discourse/app/components/user-menu/menu-tab.hbs
index 8e1e2c04eb2..860e83a12bb 100644
--- a/app/assets/javascripts/discourse/app/components/user-menu/menu-tab.hbs
+++ b/app/assets/javascripts/discourse/app/components/user-menu/menu-tab.hbs
@@ -4,6 +4,7 @@
   class={{this.classNames}}
   id={{this.id}}
   tabindex={{this.tabIndex}}
+  title={{@tab.title}}
   aria-selected={{if this.isActive "true" "false"}}
   aria-controls={{this.ariaControls}}
   data-tab-number={{@tab.position}}
@@ -11,5 +12,8 @@
   {{!-- template-lint-disable require-context-role --}}
 >
   {{d-icon @tab.icon}}
+  {{#if @tab.count}}
+    <span aria-hidden="true" class="badge-notification">{{@tab.count}}</span>
+  {{/if}}
   {{yield}}
 </button>
diff --git a/app/assets/javascripts/discourse/app/components/user-menu/menu.hbs b/app/assets/javascripts/discourse/app/components/user-menu/menu.hbs
index 2bd85ea2fc9..8663bf37d70 100644
--- a/app/assets/javascripts/discourse/app/components/user-menu/menu.hbs
+++ b/app/assets/javascripts/discourse/app/components/user-menu/menu.hbs
@@ -1,25 +1,14 @@
 <div class="user-menu revamped menu-panel drop-down" data-max-width="320" {{did-insert this.triggerRenderedAppEvent}}>
   <div class="panel-body">
     <div class="panel-body-contents">
-      <div
-        id={{concat "quick-access-" this.currentTabId}}
-        class="quick-access-panel"
-        tabindex="-1"
-        aria-labelledby={{concat "user-menu-button-" this.currentTabId}}>
-        {{component this.currentPanelComponent closeUserMenu=@closeUserMenu filterByTypes=this.currentNotificationTypes}}
-      </div>
       <div class="menu-tabs-container" role="tablist" aria-orientation="vertical" aria-label={{i18n "user_menu.sr_menu_tabs"}}>
-        <div class="top-tabs tabs-list">
+        <div class="top-tabs tabs-list" {{did-insert this.focusFirstTab}}>
           {{#each this.topTabs as |tab|}}
             <UserMenu::MenuTab
               @tab={{tab}}
               @currentTabId={{this.currentTabId}}
               @onTabClick={{fn this.handleTabClick tab}}
-            >
-              {{#if tab.count}}
-                <span class="badge-notification">{{tab.count}}</span>
-              {{/if}}
-            </UserMenu::MenuTab>
+            />
           {{/each}}
         </div>
         <div class="bottom-tabs tabs-list">
@@ -32,6 +21,17 @@
           {{/each}}
         </div>
       </div>
+      <div
+        id={{concat "quick-access-" this.currentTabId}}
+        class="quick-access-panel"
+        tabindex="-1">
+        {{component
+          this.currentPanelComponent
+          closeUserMenu=@closeUserMenu
+          filterByTypes=this.currentNotificationTypes
+          ariaLabelledby=(concat "user-menu-button-" this.currentTabId)
+        }}
+      </div>
     </div>
   </div>
 </div>
diff --git a/app/assets/javascripts/discourse/app/components/user-menu/menu.js b/app/assets/javascripts/discourse/app/components/user-menu/menu.js
index 9f882c094dc..501ebaa871e 100644
--- a/app/assets/javascripts/discourse/app/components/user-menu/menu.js
+++ b/app/assets/javascripts/discourse/app/components/user-menu/menu.js
@@ -257,7 +257,7 @@ const CORE_OTHER_NOTIFICATIONS_TAB = class extends UserMenuTab {
   }
 
   get id() {
-    return "other";
+    return "other-notifications";
   }
 
   get icon() {
@@ -378,4 +378,9 @@ export default class UserMenu extends Component {
   triggerRenderedAppEvent() {
     this.appEvents.trigger("user-menu:rendered");
   }
+
+  @action
+  focusFirstTab(topTabsContainerElement) {
+    topTabsContainerElement.querySelector(".btn.active")?.focus();
+  }
 }
diff --git a/app/assets/javascripts/discourse/app/components/user-menu/profile-tab-content.hbs b/app/assets/javascripts/discourse/app/components/user-menu/profile-tab-content.hbs
index 08eb8582801..a9158f2cc5c 100644
--- a/app/assets/javascripts/discourse/app/components/user-menu/profile-tab-content.hbs
+++ b/app/assets/javascripts/discourse/app/components/user-menu/profile-tab-content.hbs
@@ -1,4 +1,4 @@
-<ul>
+<ul aria-labelledby={{@ariaLabelledby}}>
   {{#if this.siteSettings.enable_user_status}}
     <li class="set-user-status">
       <DButton @class="btn-flat profile-tab-btn" @action={{this.setUserStatusClick}}>
diff --git a/app/assets/javascripts/discourse/app/lib/user-menu/notification-item.js b/app/assets/javascripts/discourse/app/lib/user-menu/notification-item.js
index 7660be40396..d140078e1bb 100644
--- a/app/assets/javascripts/discourse/app/lib/user-menu/notification-item.js
+++ b/app/assets/javascripts/discourse/app/lib/user-menu/notification-item.js
@@ -65,8 +65,9 @@ export default class UserMenuNotificationItem extends UserMenuBaseItem {
     if (!this.notification.read) {
       this.notification.set("read", true);
 
-      const groupedUnreadNotifications =
-        this.currentUser.grouped_unread_notifications;
+      const groupedUnreadNotifications = {
+        ...this.currentUser.grouped_unread_notifications,
+      };
       const unreadCount =
         groupedUnreadNotifications &&
         groupedUnreadNotifications[this.notification.notification_type];
diff --git a/app/assets/javascripts/discourse/app/lib/user-menu/tab.js b/app/assets/javascripts/discourse/app/lib/user-menu/tab.js
index 9e8aaa099f8..356b7f8ec1d 100644
--- a/app/assets/javascripts/discourse/app/lib/user-menu/tab.js
+++ b/app/assets/javascripts/discourse/app/lib/user-menu/tab.js
@@ -1,3 +1,5 @@
+import I18n from "I18n";
+
 /**
  * abstract class representing a tab in the user menu
  */
@@ -22,6 +24,26 @@ export default class UserMenuTab {
     return 0;
   }
 
+  /**
+   * @returns {string} title attribute for the tab element in the DOM
+   */
+  get title() {
+    const id = this.id.replaceAll(/-/g, "_");
+    const count = this.count;
+    let key;
+    if (this.count) {
+      key = `user_menu.tabs.${id}_with_unread`;
+    } else {
+      key = `user_menu.tabs.${id}`;
+    }
+    // the lookup method returns undefined if the key doesn't exist.
+    // this ensures that don't use the "missing translation" string as the
+    // title for tabs that don't define title in the yml files.
+    if (I18n.lookup(key)) {
+      return I18n.t(key, { count });
+    }
+  }
+
   /**
    * @returns {string} Dasherized version of the component name that should be rendered in the panel area when the tab is active.
    */
diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-menu-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-menu-test.js
index 000f7999e97..da24b409fd5 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/user-menu-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-menu-test.js
@@ -51,6 +51,41 @@ acceptance("User menu", function (needs) {
     requestHeaders = {};
   });
 
+  test("notifications panel has a11y attributes", async function (assert) {
+    await visit("/");
+    await click(".d-header-icons .current-user");
+    const panel = query("#quick-access-all-notifications");
+    assert.strictEqual(panel.getAttribute("tabindex"), "-1");
+    assert.strictEqual(
+      panel.querySelector("ul").getAttribute("aria-labelledby"),
+      "user-menu-button-all-notifications"
+    );
+  });
+
+  test("mentions notifications panel has a11y attributes", async function (assert) {
+    await visit("/");
+    await click(".d-header-icons .current-user");
+    await click("#user-menu-button-mentions");
+    const panel = query("#quick-access-mentions");
+    assert.strictEqual(panel.getAttribute("tabindex"), "-1");
+    assert.strictEqual(
+      panel.querySelector("ul").getAttribute("aria-labelledby"),
+      "user-menu-button-mentions"
+    );
+  });
+
+  test("profile panel has a11y attributes", async function (assert) {
+    await visit("/");
+    await click(".d-header-icons .current-user");
+    await click("#user-menu-button-profile");
+    const panel = query("#quick-access-profile");
+    assert.strictEqual(panel.getAttribute("tabindex"), "-1");
+    assert.strictEqual(
+      panel.querySelector("ul").getAttribute("aria-labelledby"),
+      "user-menu-button-profile"
+    );
+  });
+
   test("clicking on an unread notification", async function (assert) {
     await visit("/");
     await click(".d-header-icons .current-user");
@@ -133,6 +168,72 @@ acceptance("User menu", function (needs) {
     );
   });
 
+  test("tabs have title attributes", async function (assert) {
+    withPluginApi("0.1", (api) => {
+      api.registerUserMenuTab((UserMenuTab) => {
+        return class extends UserMenuTab {
+          get id() {
+            return "tiny-tab-1";
+          }
+
+          get count() {
+            return this.currentUser.get("unread_high_priority_notifications");
+          }
+
+          get icon() {
+            return "wrench";
+          }
+
+          get panelComponent() {
+            return "d-button";
+          }
+
+          get title() {
+            return `Custom title: ${this.count}`;
+          }
+        };
+      });
+    });
+
+    const expectedTitles = {
+      "user-menu-button-all-notifications": I18n.t(
+        "user_menu.tabs.all_notifications"
+      ),
+      "user-menu-button-replies": I18n.t("user_menu.tabs.replies_with_unread", {
+        count: 2,
+      }),
+      "user-menu-button-mentions": I18n.t("user_menu.tabs.mentions"),
+      "user-menu-button-likes": I18n.t("user_menu.tabs.likes"),
+      "user-menu-button-watching": I18n.t("user_menu.tabs.watching"),
+      "user-menu-button-messages": I18n.t("user_menu.tabs.messages"),
+      "user-menu-button-bookmarks": I18n.t("user_menu.tabs.bookmarks"),
+      "user-menu-button-tiny-tab-1": "Custom title: 73",
+      "user-menu-button-review-queue": I18n.t("user_menu.tabs.review_queue"),
+      "user-menu-button-other-notifications": I18n.t(
+        "user_menu.tabs.other_notifications"
+      ),
+      "user-menu-button-profile": I18n.t("user_menu.tabs.profile"),
+    };
+    await visit("/");
+    await click(".d-header-icons .current-user");
+    for (const [key, title] of Object.entries(expectedTitles)) {
+      assert.strictEqual(
+        query(`#${key}`).title,
+        title,
+        `${key} tab has the right title`
+      );
+    }
+
+    await publishToMessageBus(`/notification/${loggedInUser().id}`, {
+      unread_high_priority_notifications: 22,
+    });
+    assert.strictEqual(
+      query("#user-menu-button-tiny-tab-1").title,
+      "Custom title: 22",
+      "tabs titles can update dynamically"
+    );
+  });
+
   test("tabs added via the plugin API", async function (assert) {
     withPluginApi("0.1", (api) => {
       api.registerUserMenuTab((UserMenuTab) => {
@@ -186,7 +287,7 @@ acceptance("User menu", function (needs) {
       "user-menu-button-custom-tab-1": "7",
       "user-menu-button-custom-tab-2": "8",
       "user-menu-button-review-queue": "9",
-      "user-menu-button-other": "10",
+      "user-menu-button-other-notifications": "10",
     };
 
     await visit("/");
@@ -855,19 +956,21 @@ acceptance("User menu - Dismiss button", function (needs) {
     await visit("/");
     await click(".d-header-icons .current-user");
 
-    await click("#user-menu-button-other");
-    let repliesBadgeNotification = query(
-      "#user-menu-button-other .badge-notification"
+    await click("#user-menu-button-other-notifications");
+    let othersBadgeNotification = query(
+      "#user-menu-button-other-notifications .badge-notification"
     );
     assert.strictEqual(
-      repliesBadgeNotification.textContent.trim(),
+      othersBadgeNotification.textContent.trim(),
       "4",
       "badge shows the right count"
     );
 
     await click(".user-menu .notifications-dismiss");
 
-    assert.ok(!exists("#user-menu-button-other .badge-notification"));
+    assert.ok(
+      !exists("#user-menu-button-other-notifications .badge-notification")
+    );
     assert.ok(
       markRead,
       "mark-read request is sent without a confirmation modal"
diff --git a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js
index aad9b144938..a0de1cada5e 100644
--- a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js
@@ -21,16 +21,6 @@ module("Integration | Component | user-menu", function (hooks) {
     assert.ok(notifications[2].classList.contains("liked-consolidated"));
   });
 
-  test("notifications panel has a11y attributes", async function (assert) {
-    await render(template);
-    const panel = query("#quick-access-all-notifications");
-    assert.strictEqual(panel.getAttribute("tabindex"), "-1");
-    assert.strictEqual(
-      panel.getAttribute("aria-labelledby"),
-      "user-menu-button-all-notifications"
-    );
-  });
-
   test("active tab has a11y attributes that indicate it's active", async function (assert) {
     await render(template);
     const activeTab = query(".top-tabs.tabs-list .btn.active");
diff --git a/app/assets/stylesheets/common/base/menu-panel.scss b/app/assets/stylesheets/common/base/menu-panel.scss
index 26f4e4b7bcd..29fafc3bc48 100644
--- a/app/assets/stylesheets/common/base/menu-panel.scss
+++ b/app/assets/stylesheets/common/base/menu-panel.scss
@@ -142,7 +142,7 @@
 
   .panel-body-contents {
     display: flex;
-    flex-direction: row;
+    flex-direction: row-reverse;
   }
 
   .quick-access-panel {
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 1c9a0646507..34950b28eda 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -2567,10 +2567,45 @@ en:
     view_all: "view all %{tab}"
     user_menu:
       generic_no_items: "There are no items in this list."
-      sr_menu_tabs: "Menu tabs"
+      sr_menu_tabs: "User menu tabs"
       view_all_notifications: "view all notifications"
       view_all_bookmarks: "view all bookmarks"
       view_all_messages: "view all personal messages"
+      tabs:
+        all_notifications: "All notifications"
+        replies: "Replies"
+        replies_with_unread:
+          one: "Replies - %{count} unread reply"
+          other: "Replies - %{count} unread replies"
+        mentions: "Mentions"
+        mentions_with_unread:
+          one: "Mentions - %{count} unread mention"
+          other: "Mentions - %{count} unread mentions"
+        likes: "Likes"
+        likes_with_unread:
+          one: "Likes - %{count} unread like"
+          other: "Likes - %{count} unread likes"
+        watching: "Watched topics"
+        watching_with_unread:
+          one: "Watched topics - %{count} unread watched topic"
+          other: "Watched topics - %{count} unread watched topics"
+        messages: "Personal messages"
+        messages_with_unread:
+          one: "Personal messages - %{count} unread message"
+          other: "Personal messages - %{count} unread messages"
+        bookmarks: "Bookmarks"
+        bookmarks_with_unread:
+          one: "Bookmarks - %{count} unread bookmark"
+          other: "Bookmarks - %{count} unread bookmarks"
+        review_queue: "Review queue"
+        review_queue_with_unread:
+          one: "Review queue - %{count} item needs review"
+          other: "Review queue - %{count} items need review"
+        other_notifications: "Other notifications"
+        other_notifications_with_unread:
+          one: "Other notifications - %{count} unread notification"
+          other: "Other notifications - %{count} unread notifications"
+        profile: "Profile"
       reviewable:
         view_all: "view all review items"
         queue: "Queue"