Skip to content

dismiss editcard pressing Escape #2464

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

Merged
merged 6 commits into from
Apr 28, 2025
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
6 changes: 5 additions & 1 deletion src/app/content/highlights/components/Card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface CardProps {
highlightOffsets?: { top: number; bottom: number };
onHeightChange: (ref: React.RefObject<HTMLElement>) => void;
isHidden: boolean;
onBlurOptional?: () => void;
}

type CardPropsWithBookAndPage = Omit<CardProps, 'book' | 'page'> & {
Expand Down Expand Up @@ -81,7 +82,10 @@ function useComputedProps(props: CardProps) {
className: props.className,
highlight: props.highlight,
isActive: props.isActive,
onBlur: props.blur,
onBlur: /* istanbul ignore next */ () => {
props.blur();
if (props.onBlurOptional) props.onBlurOptional();
},
onHeightChange: props.onHeightChange,
ref: element,
shouldFocusCard: props.shouldFocusCard,
Expand Down
116 changes: 83 additions & 33 deletions src/app/content/highlights/components/CardWrapper.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const dispatchFocusOutEvent = (
};

const dispatchHighlightToggle = (target: HTMLElement | undefined) => {
dispatchKeyDownEvent({code: highlightKeyCombination.code, altKey: highlightKeyCombination.altKey, target});
dispatchKeyDownEvent({ code: highlightKeyCombination.code, altKey: highlightKeyCombination.altKey, target });
};

jest.mock('./Card', () => (props: any) => <span data-mock-card {...props} />);
Expand Down Expand Up @@ -297,52 +297,102 @@ describe('CardWrapper', () => {
});
});

it('handles useKeyCombination - hide/unhide cards', () => {
const document = assertDocument();
const highlight1 = createMockHighlight('id1');
const highlight2 = createMockHighlight('id2');
const highlightElement1 = document.createElement('span');
const highlightElement2 = document.createElement('span');
container.appendChild(highlightElement1);
container.appendChild(highlightElement2);
const component = renderer.create(
<Provider store={store}>
<CardWrapper
container={container}
highlights={[highlight1, highlight2]}
/>
</Provider>
);

const cards = component.root.findAllByType(Card);

// Expect cards to be hidden
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
dispatchKeyDownEvent({
key: 'Enter',
target: highlightElement1,
});
});

// Expect all cards to be visible
renderer.act(() => {
// Simulate pressing Tab to unhide all cards
dispatchKeyDownEvent({
key: 'Tab',
target: highlightElement1,
});
});

expect(cards[0].props.isHidden).toBe(false);
expect(cards[1].props.isHidden).toBe(false);
});

it(
'handles useKeyCombination - noop if trigerred in element that we dont support '
+ 'or with another key combination',
() => {
const document = assertDocument();
const highlight = createMockHighlight();
const highlightElement = document.createElement('span');
container.appendChild(highlightElement);
const document = assertDocument();
const highlight = createMockHighlight();
const highlightElement = document.createElement('span');
container.appendChild(highlightElement);

const textarea = document.createElement('textarea');
container.appendChild(textarea);
const textarea = document.createElement('textarea');
container.appendChild(textarea);

const elementInsideContainer = document.createElement('div');
container.appendChild(elementInsideContainer);
const elementInsideContainer = document.createElement('div');
container.appendChild(elementInsideContainer);

const elementOutsideOfTheContainer = document.createElement('div');
document.body.appendChild(elementOutsideOfTheContainer);
const elementOutsideOfTheContainer = document.createElement('div');
document.body.appendChild(elementOutsideOfTheContainer);

const cardWrapperElement = document.createElement('div');
const cardWrapperElement = document.createElement('div');

store.dispatch(focusHighlight(highlight.id));
store.dispatch(focusHighlight(highlight.id));

const component = renderer.create(<Provider store={store}>
<CardWrapper container={container} highlights={[highlight]} />
</Provider>, { createNodeMock: () => cardWrapperElement });
const component = renderer.create(<Provider store={store}>
<CardWrapper container={container} highlights={[highlight]} />
</Provider>, { createNodeMock: () => cardWrapperElement });

renderer.act(() => {
const card = component.root.findByType(Card);
expect(card.props.shouldFocusCard).toEqual(false);
});
renderer.act(() => {
const card = component.root.findByType(Card);
expect(card.props.shouldFocusCard).toEqual(false);
});

renderer.act(() => {
dispatchHighlightToggle(textarea);
renderer.act(() => {
dispatchHighlightToggle(textarea);

dispatchHighlightToggle(elementOutsideOfTheContainer);
dispatchHighlightToggle(elementOutsideOfTheContainer);

dispatchKeyDownEvent({key: 'anotherkeythatwedontsupport', target: elementInsideContainer});
});
dispatchKeyDownEvent({ key: 'anotherkeythatwedontsupport', target: elementInsideContainer });
});

renderer.act(() => {
const card = component.root.findByType(Card);
expect(card.props.shouldFocusCard).toEqual(false);
});
renderer.act(() => {
const card = component.root.findByType(Card);
expect(card.props.shouldFocusCard).toEqual(false);
});

expect(highlight.focus).not.toHaveBeenCalled();
});
expect(highlight.focus).not.toHaveBeenCalled();
});

it('handles useKeyCombination - noop if focusedHighlight is undefined', () => {
const document = assertDocument();
Expand Down Expand Up @@ -417,7 +467,7 @@ describe('CardWrapper', () => {
expect(store.getState().content.highlights.currentPage.focused).toEqual(highlight.id);

renderer.act(() => {
dispatchKeyDownEvent({key: highlightKeyCombination.key!, target: cardElement});
dispatchKeyDownEvent({ key: highlightKeyCombination.key!, target: cardElement });
});

expect(highlight.focus).not.toHaveBeenCalled();
Expand All @@ -427,7 +477,7 @@ describe('CardWrapper', () => {
it('loads', () => {
renderer.create(<Provider store={store}>
<CardWrapper container={container} highlights={[]} />
</Provider>);
</Provider>);

runHooks(renderer);

Expand Down
35 changes: 31 additions & 4 deletions src/app/content/highlights/components/CardWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { highlightKeyCombination } from '../constants';
import { focused } from '../selectors';
import Card from './Card';
import { mainWrapperStyles } from './cardStyles';
import { getHighlightOffset, noopKeyCombinationHandler, updateCardsPositions } from './cardUtils';
import { editCardVisibilityHandler, getHighlightOffset, noopKeyCombinationHandler, updateCardsPositions } from './cardUtils';

export interface WrapperProps {
hasQuery: boolean;
Expand All @@ -37,6 +37,10 @@ const Wrapper = ({highlights, className, container, highlighter}: WrapperProps)
() => highlights.find((highlight) => highlight.id === focusedId),
[focusedId, highlights]);
const setNewCardsPositionsRef = React.useRef<() => void>();
const [isHiddenByEscape, dispatch] = React.useReducer(
editCardVisibilityHandler,
new Map(highlights.map((highlight) => [highlight.id, false]))
);

// This function is triggered by keyboard shortcut defined in useKeyCombination(...)
// It moves focus between Card component and highlight in the content.
Expand All @@ -54,10 +58,32 @@ const Wrapper = ({highlights, className, container, highlighter}: WrapperProps)
}
}, [focusedHighlight, focusedId]);

const hideCard = () => {
dispatch({ type: 'HIDE', id: focusedId });
};

const showCard = () => {
dispatch({ type: 'SHOW', id: focusedId });
};

const showAllCards = () => {
dispatch({ type: 'SHOW_ALL' });
};

useKeyCombination(highlightKeyCombination, moveFocus, noopKeyCombinationHandler([container, element]));

// Allow to move back to highlight from EditCard using Escape key
useKeyCombination({key: 'Escape'}, moveFocus);
/*
* Allow to show EditCard using Enter key
* It is important to preserve the default behavior of Enter key
*/
useKeyCombination({key: 'Enter'}, showCard, undefined, false);

// Allow to hide EditCard using Escape key
useKeyCombination({key: 'Escape'}, hideCard, undefined, false);

// After move focus, reset visibility of all cards
useKeyCombination({ key: 'Tab' }, showAllCards, undefined, false);
useKeyCombination({ key: 'Tab', shiftKey: true }, showAllCards, undefined, false);

// 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.
Expand Down Expand Up @@ -128,7 +154,8 @@ const Wrapper = ({highlights, className, container, highlighter}: WrapperProps)
onHeightChange={(ref: React.RefObject<HTMLElement>) => onHeightChange(highlight.id, ref)}
zIndex={highlights.length - index}
shouldFocusCard={focusThisCard}
isHidden={checkIfHiddenByCollapsedAncestor(highlight)}
isHidden={checkIfHiddenByCollapsedAncestor(highlight) || isHiddenByEscape.get(highlight.id)}
onBlurOptional={showAllCards}
/>;
})}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ exports[`CardWrapper matches snapshot 1`] = `
}
}
isHidden={false}
onBlurOptional={[Function]}
onHeightChange={[Function]}
shouldFocusCard={false}
zIndex={1}
Expand Down
49 changes: 48 additions & 1 deletion src/app/content/highlights/components/cardUtils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Highlight } from '@openstax/highlighter';
import { HighlightColorEnum } from '@openstax/highlighter/dist/api';
import createMockHighlight from '../../../../test/mocks/highlight';
import { assertDocument } from '../../../utils';
import { generateUpdatePayload, getHighlightTopOffset } from './cardUtils';
import { generateUpdatePayload, getHighlightTopOffset, editCardVisibilityHandler } from './cardUtils';

describe('cardUtils', () => {
it('returns undefined if passed container is undefined', () => {
Expand Down Expand Up @@ -48,3 +48,50 @@ describe('generateUpdatePayload', () => {
expect(payload.updatePayload.highlight).toEqual({...update, color: oldData.color});
});
});

describe('editCardVisibilityHandler', () => {
const highlights = [
{id: 'highlight1', annotation: 'annotation1'},
{id: 'highlight2', annotation: 'annotation2'},
{id: 'highlight3', annotation: 'annotation3'},
];
const state = new Map(highlights.map((highlight) => [highlight.id, false]));

it('hide highlight', () => {
const result = editCardVisibilityHandler(state, { type: 'HIDE', id: 'highlight1'});
expect(result.get('highlight1')).toBe(true);
expect(result.get('highlight2')).toBe(false);
expect(result.get('highlight3')).toBe(false);
});

it('hide highlight - unknown id', () => {
const result = editCardVisibilityHandler(state, { type: 'HIDE'});
expect(result.get('highlight1')).toBe(false);
});

it('show highlight', () => {
const result = editCardVisibilityHandler(state, { type: 'SHOW', id: 'highlight1'});
expect(result.get('highlight1')).toBe(false);
expect(result.get('highlight2')).toBe(false);
expect(result.get('highlight3')).toBe(false);
});

it('show highlight - unknown id', () => {
const result = editCardVisibilityHandler(state, { type: 'SHOW'});
expect(result.get('highlight1')).toBe(false);
});

it('show all highlights', () => {
const firstResult = editCardVisibilityHandler(state, { type: 'HIDE', id: 'highlight1'});
expect(firstResult.get('highlight1')).toBe(true);
const secondResult = editCardVisibilityHandler(state, { type: 'SHOW_ALL'});
expect(secondResult.get('highlight1')).toBe(false);
expect(secondResult.get('highlight2')).toBe(false);
expect(secondResult.get('highlight3')).toBe(false);
});

it('return state when action type does not exist', () => {
const result = editCardVisibilityHandler(state, { type: 'SHOW_ANY'});
expect(result.get('highlight1')).toBe(false);
});
});
24 changes: 24 additions & 0 deletions src/app/content/highlights/components/cardUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,27 @@ export const noopKeyCombinationHandler = (
if (activeElement.nodeName === 'TEXTAREA') { return true; }
return false;
};

export const editCardVisibilityHandler = (state: Map<string, boolean>, action: { type: string; id?: string }) => {
switch (action.type) {
case 'SHOW': {
if (!action.id) return state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does nothing without an action.id right? Maybe it should throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is editCardVisibilityHandler will be part of CardWrapper so they are other interactive elements without action.id that can enter inot this code so throwing it would cause more troubles. That's why I just returned the state

const newState = new Map(state);
newState.set(action.id, false);
return newState;
}
case 'HIDE': {
if (!action.id) return state;
const updatedState = new Map(state);
updatedState.set(action.id, true);
return updatedState;
}
case 'SHOW_ALL': {
const allVisibleState = new Map(state);
allVisibleState.forEach((_, key) => allVisibleState.set(key, false));
return allVisibleState;
}
default:
return state;
}
};
5 changes: 3 additions & 2 deletions src/app/reactUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,8 @@ export const keyboardEventMatchesCombination = (options: KeyCombinationOptions,
export const useKeyCombination = (
options: KeyCombinationOptions,
callback: (event: KeyboardEvent) => void,
noopHandler?: (activeElement: Element) => boolean
noopHandler?: (activeElement: Element) => boolean,
preventDefault = true
) => {
const document = assertDocument();

Expand All @@ -458,7 +459,7 @@ export const useKeyCombination = (
return;
}
if (keyboardEventMatchesCombination(options, event)) {
event.preventDefault();
if (preventDefault) event.preventDefault();
callback(event);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down