From e0798b88e013f9164ac180aa38858e3249b13c55 Mon Sep 17 00:00:00 2001
From: Suzanne Aitchison <suzanne@forem.com>
Date: Tue, 15 Jun 2021 11:13:31 +0100
Subject: [PATCH] Message context menu in connect is not accessible #12250
 (#13806)

* make connect message dropdown accessible

* add a cypress test

* use crayons buttons throughout

* fix some unhandled error affecting cypress

* make message options a list

* minor tweak to test

* guard against filter error being triggered in test

* utilise newer dropdown helpers

* wait for message to fully send

* use optional chaining
---
 app/assets/stylesheets/chat.scss              | 20 +++--
 app/javascript/chat/chat.jsx                  | 32 +++-----
 app/javascript/chat/content.jsx               | 10 +--
 app/javascript/chat/message.jsx               | 79 ++++++++++++++----
 app/javascript/utilities/dropdownUtils.js     | 27 ++----
 cypress/fixtures/users/chatUser1.json         |  5 ++
 cypress/fixtures/users/chatUser2.json         |  5 ++
 .../chatFlows/messageOptions.spec.js          | 82 +++++++++++++++++++
 spec/support/seeds/seeds_e2e.rb               | 56 +++++++++++++
 9 files changed, 244 insertions(+), 72 deletions(-)
 create mode 100644 cypress/fixtures/users/chatUser1.json
 create mode 100644 cypress/fixtures/users/chatUser2.json
 create mode 100644 cypress/integration/chatFlows/messageOptions.spec.js

diff --git a/app/assets/stylesheets/chat.scss b/app/assets/stylesheets/chat.scss
index 393bde756..97b48d233 100644
--- a/app/assets/stylesheets/chat.scss
+++ b/app/assets/stylesheets/chat.scss
@@ -920,20 +920,25 @@
 }
 
 .message__actions {
-  display: none;
   position: relative;
   justify-content: end;
   -webkit-justify-content: flex-end;
 
-  .ellipsis__menubutton {
+  .ellipsis__menubutton img {
     height: 17px;
-    cursor: pointer;
+    vertical-align: middle;
     color: var(--theme-color, #0a0a0a);
   }
 
   &:hover {
-    .messagebody__dropdownmenu {
-      display: block;
+    .ellipsis__menubutton {
+      opacity: 1;
+    }
+  }
+
+  &:focus-within {
+    .ellipsis__menubutton {
+      opacity: 1;
     }
   }
 
@@ -943,15 +948,14 @@
   }
 
   .messagebody__dropdownmenu {
-    display: none;
     position: absolute;
     background-color: var(--theme-container-background, #fff);
     border-style: solid;
     border-color: var(--theme-color, #0a0a0a);
     border-radius: 5px;
     border-width: 1px;
-    margin-top: 1em;
-    margin-right: -0.6em;
+    top: 100%;
+    right: 0;
 
     button {
       width: 100%;
diff --git a/app/javascript/chat/chat.jsx b/app/javascript/chat/chat.jsx
index 57e3b0df6..719eb7d9c 100644
--- a/app/javascript/chat/chat.jsx
+++ b/app/javascript/chat/chat.jsx
@@ -664,11 +664,11 @@ export class Chat extends Component {
       });
     }
     if (messageIsEmpty) {
-      const messagesByCurrentUser = messages[activeChannelId].filter(
+      const messagesByCurrentUser = messages[activeChannelId]?.filter(
         (message) => message.user_id === currentUserId,
       );
       const lastMessage =
-        messagesByCurrentUser[messagesByCurrentUser.length - 1];
+        messagesByCurrentUser?.[messagesByCurrentUser.length - 1];
 
       if (lastMessage) {
         if (upArrowPressed) {
@@ -778,9 +778,8 @@ export class Chat extends Component {
     }
   };
   hideChannelList = () => {
-    const chatContainer = document.getElementsByClassName(
-      'chat__activechat',
-    )[0];
+    const chatContainer =
+      document.getElementsByClassName('chat__activechat')[0];
     chatContainer.classList.remove('chat__activechat--hidden');
   };
   handleSwitchChannel = (e) => {
@@ -1371,9 +1370,8 @@ export class Chat extends Component {
   };
 
   navigateToChannelsList = () => {
-    const chatContainer = document.getElementsByClassName(
-      'chat__activechat',
-    )[0];
+    const chatContainer =
+      document.getElementsByClassName('chat__activechat')[0];
 
     chatContainer.classList.add('chat__activechat--hidden');
   };
@@ -1390,12 +1388,8 @@ export class Chat extends Component {
   };
 
   handleMessageScroll = () => {
-    const {
-      allMessagesLoaded,
-      messages,
-      activeChannelId,
-      messageOffset,
-    } = this.state;
+    const { allMessagesLoaded, messages, activeChannelId, messageOffset } =
+      this.state;
 
     if (!messages[activeChannelId]) {
       return;
@@ -1608,7 +1602,7 @@ export class Chat extends Component {
     const enterPressed = e.keyCode === 13;
     if (enterPressed && showMemberlist)
       this.setState({ showMemberlist: false });
-    if (activeChannel.channel_type !== 'direct') {
+    if (activeChannel?.channel_type !== 'direct') {
       if (startEditing) {
         this.setState({ markdownEdited: true });
       }
@@ -1705,12 +1699,8 @@ export class Chat extends Component {
   };
 
   renderChannelMembersList = () => {
-    const {
-      showMemberlist,
-      activeChannelId,
-      channelUsers,
-      memberFilterQuery,
-    } = this.state;
+    const { showMemberlist, activeChannelId, channelUsers, memberFilterQuery } =
+      this.state;
 
     const filterRegx = new RegExp(memberFilterQuery, 'gi');
 
diff --git a/app/javascript/chat/content.jsx b/app/javascript/chat/content.jsx
index f69db256c..96e297f15 100644
--- a/app/javascript/chat/content.jsx
+++ b/app/javascript/chat/content.jsx
@@ -39,12 +39,8 @@ export class Content extends Component {
   };
 
   render() {
-    const {
-      onTriggerContent,
-      fullscreen,
-      resource,
-      closeReportAbuseForm,
-    } = this.props;
+    const { onTriggerContent, fullscreen, resource, closeReportAbuseForm } =
+      this.props;
     if (!resource) {
       return '';
     }
@@ -52,13 +48,13 @@ export class Content extends Component {
     return (
       // TODO: A button (role="button") cannot contain other interactive elements, i.e. buttons.
       // TODO: These should have key click events as well.
+      // eslint-disable-next-line jsx-a11y/click-events-have-key-events
       <div
         className="activechatchannel__activecontent activechatchannel__activecontent--sidecar"
         id="chat_activecontent"
         onClick={onTriggerContent}
         role="button"
         tabIndex="0"
-        aria-hidden="true"
       >
         <button
           type="button"
diff --git a/app/javascript/chat/message.jsx b/app/javascript/chat/message.jsx
index be71a5051..07dedc595 100644
--- a/app/javascript/chat/message.jsx
+++ b/app/javascript/chat/message.jsx
@@ -1,10 +1,12 @@
 import { h } from 'preact';
+import { useState, useRef, useLayoutEffect } from 'preact/hooks';
 import PropTypes from 'prop-types';
 // eslint-disable-next-line import/no-unresolved
 import ThreeDotsIcon from 'images/overflow-horizontal.svg';
 import { adjustTimestamp } from './util';
 import { ErrorMessage } from './messages/errorMessage';
 import { Button } from '@crayons';
+import { initializeDropdown } from '@utilities/dropdownUtils';
 
 export const Message = ({
   currentUserId,
@@ -22,8 +24,22 @@ export const Message = ({
   onReportMessageTrigger,
   onEditMessageTrigger,
 }) => {
+  const [isDropdownOpen, setIsDropdownOpen] = useState(false);
+  const messageWrapperRef = useRef(null);
   const spanStyle = { color };
 
+  const triggerElementId = `message-dropdown-trigger-${id}`;
+  const dropdownContentId = `message-dropdown-${id}`;
+
+  useLayoutEffect(() => {
+    initializeDropdown({
+      triggerElementId,
+      dropdownContentId,
+      onOpen: () => setIsDropdownOpen(true),
+      onClose: () => setIsDropdownOpen(false),
+    });
+  }, [triggerElementId, dropdownContentId]);
+
   if (type === 'error') {
     return <ErrorMessage message={message} />;
   }
@@ -44,30 +60,57 @@ export const Message = ({
 
   const dropdown = (
     <div className="message__actions">
-      <span className="ellipsis__menubutton">
-        <img src={ThreeDotsIcon} alt="dropdown menu icon" />
-      </span>
+      <Button
+        id={triggerElementId}
+        className={`ellipsis__menubutton ${
+          isDropdownOpen ? 'opacity-1' : 'opacity-0'
+        }`}
+        size="s"
+        variant="ghost"
+      >
+        <img src={ThreeDotsIcon} alt="Message options menu" />
+      </Button>
 
-      <div className="messagebody__dropdownmenu">
-        <Button variant="ghost" onClick={(_) => onEditMessageTrigger(id)}>
-          Edit
-        </Button>
-        <Button
-          variant="ghost-danger"
-          onClick={(_) => onDeleteMessageTrigger(id)}
-        >
-          Delete
-        </Button>
+      <div id={dropdownContentId} className="messagebody__dropdownmenu hidden">
+        <ul class="list-none">
+          <li>
+            <Button
+              id={`edit-button-${id}`}
+              variant="ghost"
+              onClick={(_) => onEditMessageTrigger(id)}
+            >
+              Edit
+            </Button>
+          </li>
+          <li>
+            <Button
+              variant="ghost-danger"
+              onClick={(_) => onDeleteMessageTrigger(id)}
+            >
+              Delete
+            </Button>
+          </li>
+        </ul>
       </div>
     </div>
   );
   const dropdownReport = (
     <div className="message__actions">
-      <span className="ellipsis__menubutton">
-        <img src={ThreeDotsIcon} alt="message actions" />
-      </span>
+      <Button
+        id={triggerElementId}
+        className={`ellipsis__menubutton ${
+          isDropdownOpen ? 'opacity-1' : 'opacity-0'
+        }`}
+        size="s"
+        variant="ghost"
+      >
+        <img src={ThreeDotsIcon} alt="Report message options" />
+      </Button>
 
-      <div className="messagebody__dropdownmenu report__abuse__button">
+      <div
+        id={dropdownContentId}
+        className={`messagebody__dropdownmenu report__abuse__button hidden`}
+      >
         <Button
           variant="ghost-danger"
           onClick={(_) => onReportMessageTrigger(id)}
@@ -79,7 +122,7 @@ export const Message = ({
   );
 
   return (
-    <div className="chatmessage">
+    <div ref={messageWrapperRef} className="chatmessage">
       <div className="chatmessage__profilepic">
         <a
           href={`/${user}`}
diff --git a/app/javascript/utilities/dropdownUtils.js b/app/javascript/utilities/dropdownUtils.js
index 894a4e090..c02e43e8c 100644
--- a/app/javascript/utilities/dropdownUtils.js
+++ b/app/javascript/utilities/dropdownUtils.js
@@ -14,11 +14,7 @@ const INTERACTIVE_ELEMENTS_QUERY =
  * @param {string} args.dropdownContentId The id of the dropdown content element
  * @param {Function} args.onClose Optional function for any side-effects which should occur on dropdown close
  */
-const keyUpListener = ({
-  triggerElementId,
-  dropdownContentId,
-  onClose = () => {},
-}) => {
+const keyUpListener = ({ triggerElementId, dropdownContentId, onClose }) => {
   return ({ key }) => {
     if (key === 'Escape') {
       // Close the dropdown and return focus to the trigger button to prevent focus being lost
@@ -53,7 +49,7 @@ const keyUpListener = ({
 const clickOutsideListener = ({
   triggerElementId,
   dropdownContentId,
-  onClose = () => {},
+  onClose,
 }) => {
   return ({ target }) => {
     const triggerElement = document.getElementById(triggerElementId);
@@ -86,11 +82,7 @@ const clickOutsideListener = ({
  * @param {string} args.dropdownContent The id of the dropdown content element
  * @param {Function} args.onClose Optional function for any side-effects which should occur on dropdown close
  */
-const openDropdown = ({
-  triggerElementId,
-  dropdownContentId,
-  onClose = () => {},
-}) => {
+const openDropdown = ({ triggerElementId, dropdownContentId, onClose }) => {
   const dropdownContent = document.getElementById(dropdownContentId);
   const triggerElement = document.getElementById(triggerElementId);
 
@@ -121,11 +113,7 @@ const openDropdown = ({
  * @param {string} args.dropdownContent The id of the dropdown content element
  * @param {Function} args.onClose Optional function for any side-effects which should occur on dropdown close
  */
-const closeDropdown = ({
-  triggerElementId,
-  dropdownContentId,
-  onClose = () => {},
-}) => {
+const closeDropdown = ({ triggerElementId, dropdownContentId, onClose }) => {
   const dropdownContent = document.getElementById(dropdownContentId);
 
   document
@@ -143,7 +131,7 @@ const closeDropdown = ({
     'click',
     clickOutsideListener({ triggerElementId, dropdownContentId, onClose }),
   );
-  onClose();
+  onClose?.();
 };
 
 /**
@@ -154,13 +142,15 @@ const closeDropdown = ({
  * @param {string} args.triggerButtonElementId The ID of the button which triggers the dropdown open/close behavior
  * @param {string} args.dropdownContentId The ID of the dropdown content which should open/close on trigger button press
  * @param {Function} args.onClose An optional callback for when the dropdown is closed. This can be passed to execute any side-effects required when the dropdown closes.
+ * @param {Function} args.onOpen An optional callback for when the dropdown is opened. This can be passed to execute any side-effects required when the dropdown opens.
  *
  * @returns {{closeDropdown: Function}} Object with callback to close the initialized dropdown
  */
 export const initializeDropdown = ({
   triggerElementId,
   dropdownContentId,
-  onClose = () => {},
+  onClose,
+  onOpen,
 }) => {
   const triggerButton = document.getElementById(triggerElementId);
   const dropdownContent = document.getElementById(dropdownContentId);
@@ -192,6 +182,7 @@ export const initializeDropdown = ({
         dropdownContentId,
         onClose,
       });
+      onOpen?.();
     }
   });
 
diff --git a/cypress/fixtures/users/chatUser1.json b/cypress/fixtures/users/chatUser1.json
new file mode 100644
index 000000000..2224f4445
--- /dev/null
+++ b/cypress/fixtures/users/chatUser1.json
@@ -0,0 +1,5 @@
+{
+  "username": "chat_user_1",
+  "email": "chat-user-1@forem.local",
+  "password": "password"
+}
diff --git a/cypress/fixtures/users/chatUser2.json b/cypress/fixtures/users/chatUser2.json
new file mode 100644
index 000000000..343dfea0d
--- /dev/null
+++ b/cypress/fixtures/users/chatUser2.json
@@ -0,0 +1,5 @@
+{
+  "username": "chat_user_2",
+  "email": "chat-user-2@forem.local",
+  "password": "password"
+}
diff --git a/cypress/integration/chatFlows/messageOptions.spec.js b/cypress/integration/chatFlows/messageOptions.spec.js
new file mode 100644
index 000000000..4858f7fd5
--- /dev/null
+++ b/cypress/integration/chatFlows/messageOptions.spec.js
@@ -0,0 +1,82 @@
+describe('Chat message options', () => {
+  beforeEach(() => {
+    cy.testSetup();
+    cy.intercept(
+      { method: 'POST', url: '/chat_channels/1/open' },
+      { body: {} },
+    );
+
+    cy.fixture('users/chatUser1.json').as('user');
+    cy.fixture('users/chatUser2.json').as('user2');
+
+    cy.get('@user').then((user) => {
+      cy.loginUser(user).then(() => {
+        cy.visit('/connect');
+      });
+    });
+  });
+
+  it('should show message option menus', () => {
+    //   Enter the test chat
+    cy.findByRole('button', { name: 'Toggle request manager' }).click();
+    cy.findByRole('button', { name: 'Accept' }).click();
+    // Wait for acceptance to happen
+    cy.findByRole('button', { name: 'Accept' }).should('not.exist');
+
+    // Send a message
+    cy.findByRole('textbox', { name: 'Compose a message' })
+      .click()
+      .focus()
+      .type('message');
+    cy.findByRole('button', { name: 'Send' }).click();
+    // Wait for the message to send
+    cy.findByRole('textbox', { name: 'Compose a message' }).should(
+      'have.value',
+      '',
+    );
+
+    // The sent message doesn't show up without reload
+    cy.reload();
+
+    // Check the menu opens and focuses the first item
+    cy.findByRole('button', { name: 'Message options menu' }).as(
+      'optionsMenuButton',
+    );
+    cy.get('@optionsMenuButton').click();
+    cy.findByRole('button', { name: 'Edit' }).should('have.focus');
+    cy.findByRole('button', { name: 'Delete' }).should('exist');
+
+    // Simulate an escape keypress anywhere on the page
+    cy.get('body').type('{esc}');
+    cy.get('@optionsMenuButton').should('have.focus');
+    cy.findByRole('button', { name: 'Edit' }).should('not.exist');
+    cy.findByRole('button', { name: 'Delete' }).should('not.exist');
+
+    // Log out the current user
+    cy.findByText('Sign Out').click({ force: true });
+    cy.findByRole('button', { name: 'Yes, sign out' }).click();
+
+    // Log in as someone else to verify the report abuse options
+    cy.get('@user2').then((user) => {
+      cy.loginUser(user).then(() => {
+        //   Enter the test chat
+        cy.visit('/connect');
+        cy.findByRole('button', { name: 'Toggle request manager' }).click();
+        cy.findByRole('button', { name: 'Accept' }).click();
+        cy.findByRole('button', { name: 'Accept' }).should('not.exist');
+
+        // Check the report menu is present and opens
+        cy.findByRole('button', { name: 'Report message options' }).as(
+          'reportMenuButton',
+        );
+        cy.get('@reportMenuButton').click();
+        cy.findByRole('button', { name: 'Report Abuse' }).should('have.focus');
+
+        // Check Escape closes the menu
+        cy.get('body').type('{esc}');
+        cy.get('@reportMenuButton').should('have.focus');
+        cy.findByRole('button', { name: 'Report Abuse' }).should('not.exist');
+      });
+    });
+  });
+});
diff --git a/spec/support/seeds/seeds_e2e.rb b/spec/support/seeds/seeds_e2e.rb
index adc03bb95..b6b7b90c4 100644
--- a/spec/support/seeds/seeds_e2e.rb
+++ b/spec/support/seeds/seeds_e2e.rb
@@ -116,6 +116,62 @@ end
 
 ##############################################################################
 
+chat_user_1 = seeder.create_if_doesnt_exist(User, "email", "chat-user-1@forem.local") do
+  User.create!(
+    name: "Chat user 1",
+    email: "chat-user-1@forem.local",
+    username: "chat_user_1",
+    summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false),
+    profile_image: File.open(Rails.root.join("app/assets/images/#{rand(1..40)}.png")),
+    website_url: Faker::Internet.url,
+    email_comment_notifications: false,
+    email_follower_notifications: false,
+    confirmed_at: Time.current,
+    password: "password",
+    password_confirmation: "password",
+    saw_onboarding: true,
+    checked_code_of_conduct: true,
+    checked_terms_and_conditions: true,
+  )
+end
+
+##############################################################################
+
+chat_user_2 = seeder.create_if_doesnt_exist(User, "email", "chat-user-2@forem.local") do
+  User.create!(
+    name: "Chat user 2",
+    email: "chat-user-2@forem.local",
+    username: "chat_user_2",
+    summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false),
+    profile_image: File.open(Rails.root.join("app/assets/images/#{rand(1..40)}.png")),
+    website_url: Faker::Internet.url,
+    email_comment_notifications: false,
+    email_follower_notifications: false,
+    confirmed_at: Time.current,
+    password: "password",
+    password_confirmation: "password",
+    saw_onboarding: true,
+    checked_code_of_conduct: true,
+    checked_terms_and_conditions: true,
+  )
+end
+
+##############################################################################
+
+seeder.create_if_doesnt_exist(ChatChannel, "channel_name", "test chat channel") do
+  channel = ChatChannel.create(
+    channel_type: "open",
+    channel_name: "test chat channel",
+    slug: "test-chat-channel",
+    last_message_at: 1.week.ago,
+    status: "active",
+  )
+
+  channel.invite_users(users: [chat_user_1, chat_user_2])
+end
+
+##############################################################################
+
 seeder.create_if_none(NavigationLink) do
   protocol = ApplicationConfig["APP_PROTOCOL"].freeze
   domain = Rails.application&.initialized? ? Settings::General.app_domain : ApplicationConfig["APP_DOMAIN"]