Core-20: clear selection when user tab-navigates (#2578)

* Core-20: clear selection when user tab-navigates

[CORE-20]

* Move "HiddenOnMobile" up the hierarchy

Update EditCard.spec.tsx.snap

* Highlights basically work

* CORE_1319

Update CardWrapper.tsx

* CORE-1318

* Tests

* Use Intl for message

* Separate message for create or edit

Author: RoyEJohnson

src/app/content/highlights/components/CardWrapper.spec.tsx

@@ -138,7 +138,7 @@ describe('CardWrapper', () => {
 
     // Handle null value
     renderer.act(() => {
-      card1.props.onHeightChange({ current: { offsetHeight: null } });
+      card1.props.onHeightChange({ current: null });
     });
     // First card have null height so secondcard starts at the highlight top offset
     expect(card1.props.topOffset).toEqual(100);
@@ -332,6 +332,14 @@ describe('CardWrapper', () => {
       });
     });
 
+    renderer.act(() => {
+      // Simulate pressing Escape to hide card
+      dispatchKeyDownEvent({
+        key: 'Escape',
+        target: highlightElement1,
+      });
+    });
+
     // Expect cards to be visible again
     renderer.act(() => {
       // Simulate pressing Enter to unhide cards
@@ -341,7 +349,6 @@ describe('CardWrapper', () => {
       });
     });
 
-    // Expect all cards to be visible
     renderer.act(() => {
       // Simulate pressing Tab to unhide all cards
       dispatchKeyDownEvent({
@@ -350,11 +357,22 @@ describe('CardWrapper', () => {
       });
     });
 
-    // Set focusedHighlight, and do double=click
     renderer.act(() => {
-      highlightElement1.dispatchEvent(new Event('focus', { bubbles: true }));
-      highlightElement1.dispatchEvent(new Event('dblclick', { bubbles: true }));
+      // Simulate pressing Escape to hide card
+      dispatchKeyDownEvent({
+        key: 'Escape',
+        target: highlightElement1,
+      });
     });
+
+    // Trigger editOnEnter with no focusedHighlight
+    renderer.act(() => {
+      dispatchKeyDownEvent({
+          key: 'Enter',
+      });
+    });
+
+    document?.dispatchEvent(new CustomEvent('showCardEvent', { bubbles: true }));
   });
 
   it(

src/app/content/highlights/components/CardWrapper.tsx

@@ -6,7 +6,7 @@ import ResizeObserver from 'resize-observer-polyfill';
 import styled from 'styled-components';
 import { isHtmlElement } from '../../../guards';
 import { useFocusLost, useKeyCombination, useFocusHighlight } from '../../../reactUtils';
-import { AppState } from '../../../types';
+import { AppState, Dispatch } from '../../../types';
 import { assertDefined, assertDocument } from '../../../utils';
 import * as selectSearch from '../../search/selectors';
 import * as contentSelect from '../../selectors';
@@ -23,6 +23,7 @@ export interface WrapperProps {
   highlighter: Highlighter;
   highlights: Highlight[];
   className?: string;
+  dispatch: Dispatch;
 }
 
 function checkIfHiddenByCollapsedAncestor(highlight: Highlight) {
@@ -87,12 +88,6 @@ function useCardsHeights() {
   return [cardsHeights, onHeightChange] as const;
 }
 
-function rangeString({startOffset, endOffset}: Highlight['range']) {
-  return `${startOffset}-${endOffset}`;
-}
-
-const ELAPSED_LIMIT = 100;
-
 function useFocusedHighlight(
   highlights: Highlight[],
   element: React.RefObject<HTMLElement>,
@@ -104,47 +99,23 @@ function useFocusedHighlight(
     [focusedId, highlights]);
   const [shouldFocusCard, setShouldFocusCard] = React.useState(false);
   const document = assertDocument();
-  const previousRange = React.useRef<string>('');
-  const [dblclickStamp, setDblclickStamp] = React.useState<number>(0);
 
+  // catches the "click here" event sent by the EditCard
   React.useEffect(() => {
-    const handler = () => setDblclickStamp(Date.now());
+    const handler = () => setShouldFocusCard(true);
 
-    document.addEventListener('dblclick', handler);
-    return () => document.removeEventListener('dblclick', handler);
+    document.addEventListener('showCardEvent', handler);
+    return () => document.removeEventListener('showCardEvent', handler);
   }, [document]);
 
-  // Tracking double clicks
-  // double-click events trigger updates to focusedHighlight, but the order and
-  // timing of processing can vary, so we check conditions within a short time
-  // of a double click.
+  // Ensure focusedHighlight is actually focused
   React.useEffect(() => {
-    if (focusedHighlight) {
-      const elapsed = Date.now() - dblclickStamp;
-      const isDoubleClick = elapsed < ELAPSED_LIMIT;
-
-      // Existing highlight
-      if (focusedHighlight.elements.length > 0) {
-        if (isDoubleClick) {
-          // Unselect text inside existing highlight.
-          document.getSelection()?.removeAllRanges();
-          setShouldFocusCard(true);
-        }
-        return;
-      }
-
-      // Text selection that could be a highlight
-      const newRange = rangeString(focusedHighlight.range);
-      const isExistingSelection = newRange === previousRange.current;
-
-      if (isExistingSelection && isDoubleClick) {
-        setShouldFocusCard(true);
-      }
-      previousRange.current = newRange;
+    if (focusedHighlight && focusedHighlight.elements.length > 0) {
+      focusedHighlight?.focus();
     }
-  }, [focusedHighlight, document, dblclickStamp]);
+  }, [focusedHighlight]);
 
-  // Let Enter go from a highlight to the editor
+  // Pressing Enter moves the users from a highlight to the editor
   const editOnEnter = React.useCallback(() => {
     if (focusedHighlight) {
       setShouldFocusCard(true);
@@ -163,20 +134,23 @@ function useFocusedHighlight(
     setShouldFocusCard(!cardIsFocused);
   }, [element, focusedHighlight]);
 
-  useKeyCombination({key: 'Enter'}, editOnEnter);
+  useKeyCombination({key: 'Enter'}, editOnEnter, noopKeyCombinationHandler([container, element]));
   useKeyCombination(highlightKeyCombination, moveFocus, noopKeyCombinationHandler([container, element]));
   // Clear shouldFocusCard when focus is lost from the CardWrapper.
   // If we don't do this then card related for the focused highlight will be focused automatically.
   useFocusLost(element, shouldFocusCard, React.useCallback(() => setShouldFocusCard(false), []));
 
-  return [focusedHighlight, shouldFocusCard] as const;
+  return [focusedHighlight, shouldFocusCard, setShouldFocusCard] as const;
 }
 
-function CardsForHighlights({highlights, container, focusedHighlight, shouldFocusCard, highlighter}: {
+function CardsForHighlights({
+  highlights, container, focusedHighlight, shouldFocusCard, setShouldFocusCard, highlighter,
+}: {
   highlights: Highlight[];
   container: HTMLElement;
   focusedHighlight: Highlight | undefined;
   shouldFocusCard: boolean;
+  setShouldFocusCard: (v: boolean) => void;
   highlighter: Highlighter;
 }) {
   const [cardsHeights, onHeightChange] = useCardsHeights();
@@ -190,9 +164,18 @@ function CardsForHighlights({highlights, container, focusedHighlight, shouldFocu
     editCardVisibilityHandler,
     new Map(highlights.map((highlight) => [highlight.id, false]))
   );
+
+  // First time, Esc closes it to the instructions; second Esc disappears it
   const hideCard = () => {
+    if (!focusedHighlight?.elements.length) {
+      return;
+    }
     focusedHighlight?.focus();
-    dispatch({ type: 'HIDE', id: focusedHighlight?.id });
+    if (shouldFocusCard) {
+      setShouldFocusCard(false);
+    } else {
+      dispatch({ type: 'HIDE', id: focusedHighlight?.id });
+    }
   };
   const showCard = (cardId: string | undefined) => {
     dispatch({ type: 'SHOW', id: cardId });
@@ -228,14 +211,15 @@ function CardsForHighlights({highlights, container, focusedHighlight, shouldFocu
 // tslint:disable-next-line:variable-name
 const Wrapper = ({highlights, className, container, highlighter}: WrapperProps) => {
   const element = React.useRef<HTMLElement>(null);
-  const [focusedHighlight, shouldFocusCard] = useFocusedHighlight(highlights, element, container);
+  const [focusedHighlight, shouldFocusCard, setShouldFocusCard] = useFocusedHighlight(highlights, element, container);
 
   return <div className={className} ref={element}>
     <CardsForHighlights
       highlights={highlights}
       container={container}
       focusedHighlight={focusedHighlight}
       shouldFocusCard={shouldFocusCard}
+      setShouldFocusCard={setShouldFocusCard}
       highlighter={highlighter}
     />
   </div>;

src/app/content/highlights/components/EditCard.spec.tsx

@@ -2,6 +2,7 @@ import { Highlight } from '@openstax/highlighter';
 import { HighlightUpdateColorEnum } from '@openstax/highlighter/dist/api';
 import React, { ReactElement } from 'react';
 import renderer from 'react-test-renderer';
+import ReactTestUtils from 'react-dom/test-utils';
 import createTestServices from '../../../../test/createTestServices';
 import createTestStore from '../../../../test/createTestStore';
 import createMockHighlight from '../../../../test/mocks/highlight';
@@ -101,6 +102,27 @@ describe('EditCard', () => {
 
     const tree = component.toJSON();
     expect(tree).toMatchSnapshot();
+
+    mockSpyUser.mockClear();
+  });
+
+  it('shows create highlight message for new highlight', () => {
+    const newHighlight = {...highlight, elements: []};
+    const mockSpyUser = jest.spyOn(selectAuth, 'user')
+      .mockReturnValue(formatUser(testAccountsUser));
+    const component = renderer.create(
+      <TestContainer services={services} store={store}>
+        <EditCard
+          {...{...editCardProps, highlight: newHighlight}}
+          data={highlightData}
+          isActive={true}
+          shouldFocusCard={false}
+        />
+      </TestContainer>
+    );
+
+    const button = component.root.findByType('button');
+    expect(button.props.children.props.id).toBe('i18n:highlighting:create-instructions');
     mockSpyUser.mockClear();
   });
 
@@ -629,7 +651,7 @@ describe('EditCard', () => {
       ...highlightData,
     };
 
-    renderToDom(
+    const component = renderToDom(
       <div id={MAIN_CONTENT_ID} tabIndex={-1}>
         <TestContainer services={services} store={store}>
           <a href='#foo'>text</a>
@@ -650,6 +672,17 @@ describe('EditCard', () => {
     document?.querySelector('a')?.focus();
     document?.getElementById(MAIN_CONTENT_ID)?.focus();
     expect(editCardProps.onBlur).not.toHaveBeenCalled();
+    const button = component.node.querySelector('button') as HTMLButtonElement;
+    const preventDefault = jest.fn();
+    document!.dispatchEvent = jest.fn();
+
+    // Two branches of showCard - must be mousedown of button 0
+    ReactTestUtils.Simulate.mouseDown(button, { preventDefault, button: 1 });
+    expect(preventDefault).not.toHaveBeenCalled();
+    ReactTestUtils.Simulate.mouseDown(button, { preventDefault, button: 0 });
+    expect(preventDefault).toHaveBeenCalled();
+    expect(document!.dispatchEvent).toHaveBeenCalled();
+
     mockSpyUser.mockClear();
   });
 

src/app/content/highlights/components/EditCard.tsx

@@ -12,6 +12,7 @@ import { useFocusElement, useOnEsc, useTrapTabNavigation } from '../../../reactU
 import theme from '../../../theme';
 import { assertDefined, assertWindow, mergeRefs } from '../../../utils';
 import { highlightStyles } from '../../constants';
+import { cardWidth } from '../constants';
 import defer from 'lodash/fp/defer';
 import {
   clearFocusedHighlight,
@@ -67,6 +68,13 @@ function LoginOrEdit({
   const authenticated = !!useSelector(selectAuth.user);
   const element = React.useRef<HTMLElement>(null);
   const {formatMessage} = useIntl();
+  const showCard = React.useCallback((event: React.MouseEvent) => {
+    if (event.button === 0) {
+      event.preventDefault();
+      document?.dispatchEvent(new CustomEvent('showCardEvent', { bubbles: true }));
+    }
+  }, []);
+  const isNewSelection = props.highlight.elements.length === 0;
 
   return (
     <div
@@ -76,16 +84,26 @@ function LoginOrEdit({
     >
       {
         authenticated ? (
-          (props.shouldFocusCard || props.data?.annotation) ? (
-            <form
-              ref={mergeRefs(fref, element)}
-              data-analytics-region='edit-note'
-              data-highlight-card
-            >
-              <ActiveEditCard props={props} element={element} />
-            </form>
-          ) :
-          <i>Press Enter or double-click highlight to edit highlight</i>
+          <HiddenOnMobile>
+            {
+              (props.shouldFocusCard || props.data?.annotation) ? (
+                <form
+                  ref={mergeRefs(fref, element)}
+                  data-analytics-region='edit-note'
+                  data-highlight-card
+                >
+                  <ActiveEditCard props={props} element={element} />
+                </form>
+              ) :
+              <button type='button' onMouseDown={showCard}>
+                <FormattedMessage id={
+                  isNewSelection
+                    ? 'i18n:highlighting:create-instructions'
+                    : 'i18n:highlighting:instructions'
+                } />
+              </button>
+            }
+          </HiddenOnMobile>
         ) : <LoginConfirmation onBlur={props.onBlur} />
       }
     </div>
@@ -117,6 +135,7 @@ function LoginConfirmation({
 
 // tslint:disable-next-line:variable-name
 const HiddenOnMobile = styled.div`
+  min-width: ${cardWidth}rem;
   ${theme.breakpoints.touchDeviceQuery(css`
     display: none;
   `)}
@@ -205,7 +224,7 @@ function ActiveEditCard({
   useTrapTabNavigation(ref, editingAnnotation);
 
   return (
-    <HiddenOnMobile ref={ref}>
+    <div ref={ref}>
       <ColorPicker
         color={props.data?.color}
         onChange={onColorChange}
@@ -248,7 +267,7 @@ function ActiveEditCard({
           always={() => setConfirmingDelete(false)}
         />
       )}
-    </HiddenOnMobile>
+    </div>
   );
 }