From eb203ba929d5cb8e12f2ec7ff83ea54cb62cef97 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Fri, 12 Feb 2021 15:58:14 -0600 Subject: [PATCH] Disable search keyboard shortcuts when main header isn't shown --- ts/background.ts | 37 +--------- ts/components/MainHeader.stories.tsx | 5 +- ts/components/MainHeader.tsx | 68 +++++++++++++++---- ts/state/ducks/conversations.ts | 22 +++--- ts/state/selectors/conversations.ts | 26 ++++++- ts/state/selectors/search.ts | 4 +- ts/state/smart/LeftPane.tsx | 4 +- ts/state/smart/MainHeader.tsx | 3 +- .../state/selectors/conversations_test.ts | 63 +++++++++++++++++ ts/util/lint/exceptions.json | 4 +- 10 files changed, 166 insertions(+), 70 deletions(-) diff --git a/ts/background.ts b/ts/background.ts index 3549b9510..1469ad78d 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -948,10 +948,9 @@ type WhatIsThis = import('./window.d').WhatIsThis; const commandKey = window.platform === 'darwin' && metaKey; const controlKey = window.platform !== 'darwin' && ctrlKey; const commandOrCtrl = commandKey || controlKey; - const commandAndCtrl = commandKey && ctrlKey; const state = store.getState(); - const selectedId = state.conversations.selectedConversation; + const selectedId = state.conversations.selectedConversationId; const conversation = window.ConversationController.get(selectedId); const isSearching = window.Signal.State.Selectors.search.isSearching( state @@ -1265,40 +1264,6 @@ type WhatIsThis = import('./window.d').WhatIsThis; return; } - // Search - if ( - commandOrCtrl && - !commandAndCtrl && - !shiftKey && - (key === 'f' || key === 'F') - ) { - const { startSearch } = actions.search; - startSearch(); - - event.preventDefault(); - event.stopPropagation(); - return; - } - - // Search in conversation - if ( - conversation && - commandOrCtrl && - !commandAndCtrl && - shiftKey && - (key === 'f' || key === 'F') - ) { - const { searchInConversation } = actions.search; - const name = conversation.isMe() - ? window.i18n('noteToSelf') - : conversation.getTitle(); - searchInConversation(conversation.id, name); - - event.preventDefault(); - event.stopPropagation(); - return; - } - // Focus composer field if ( conversation && diff --git a/ts/components/MainHeader.stories.tsx b/ts/components/MainHeader.stories.tsx index e54303e40..42bae1449 100644 --- a/ts/components/MainHeader.stories.tsx +++ b/ts/components/MainHeader.stories.tsx @@ -33,6 +33,7 @@ const createProps = (overrideProps: Partial = {}): PropsType => ({ 'searchConversationId', overrideProps.searchConversationId ), + selectedConversation: undefined, startSearchCounter: 0, ourConversationId: '', @@ -46,10 +47,12 @@ const createProps = (overrideProps: Partial = {}): PropsType => ({ avatarPath: optionalText('avatarPath', overrideProps.avatarPath), i18n, + updateSearchTerm: action('updateSearchTerm'), searchMessages: action('searchMessages'), searchDiscussions: action('searchDiscussions'), - + startSearch: action('startSearch'), + searchInConversation: action('searchInConversation'), clearConversationSearch: action('clearConversationSearch'), clearSearch: action('clearSearch'), diff --git a/ts/components/MainHeader.tsx b/ts/components/MainHeader.tsx index fdb57856a..e0c719855 100644 --- a/ts/components/MainHeader.tsx +++ b/ts/components/MainHeader.tsx @@ -12,12 +12,14 @@ import { Avatar } from './Avatar'; import { AvatarPopup } from './AvatarPopup'; import { LocalizerType } from '../types/Util'; import { ColorType } from '../types/Colors'; +import { ConversationType } from '../state/ducks/conversations'; export type PropsType = { searchTerm: string; searchConversationName?: string; searchConversationId?: string; startSearchCounter: number; + selectedConversation: undefined | ConversationType; // To be used as an ID ourConversationId: string; @@ -36,7 +38,10 @@ export type PropsType = { avatarPath?: string; i18n: LocalizerType; + updateSearchTerm: (searchTerm: string) => void; + startSearch: () => void; + searchInConversation: (id: string, name: string) => void; searchMessages: ( query: string, options: { @@ -53,7 +58,6 @@ export type PropsType = { noteToSelf: string; } ) => void; - clearConversationSearch: () => void; clearSearch: () => void; @@ -107,12 +111,6 @@ export class MainHeader extends React.Component { } }; - public handleOutsideKeyDown = (event: KeyboardEvent): void => { - if (event.key === 'Escape') { - this.hideAvatarPopup(); - } - }; - public showAvatarPopup = (): void => { const popperRoot = document.createElement('div'); document.body.appendChild(popperRoot); @@ -122,14 +120,12 @@ export class MainHeader extends React.Component { popperRoot, }); document.addEventListener('click', this.handleOutsideClick); - document.addEventListener('keydown', this.handleOutsideKeyDown); }; public hideAvatarPopup = (): void => { const { popperRoot } = this.state; document.removeEventListener('click', this.handleOutsideClick); - document.removeEventListener('keydown', this.handleOutsideKeyDown); this.setState({ showingAvatarPopup: false, @@ -141,11 +137,15 @@ export class MainHeader extends React.Component { } }; + public componentDidMount(): void { + document.addEventListener('keydown', this.handleGlobalKeyDown); + } + public componentWillUnmount(): void { const { popperRoot } = this.state; document.removeEventListener('click', this.handleOutsideClick); - document.removeEventListener('keydown', this.handleOutsideKeyDown); + document.removeEventListener('keydown', this.handleGlobalKeyDown); if (popperRoot && document.body.contains(popperRoot)) { document.body.removeChild(popperRoot); @@ -225,7 +225,7 @@ export class MainHeader extends React.Component { this.setFocus(); }; - public handleKeyDown = ( + public handleInputKeyDown = ( event: React.KeyboardEvent ): void => { const { @@ -262,6 +262,50 @@ export class MainHeader extends React.Component { event.stopPropagation(); }; + public handleGlobalKeyDown = (event: KeyboardEvent): void => { + const { showingAvatarPopup } = this.state; + const { + i18n, + selectedConversation, + startSearch, + searchInConversation, + } = this.props; + + const { ctrlKey, metaKey, shiftKey, key } = event; + const commandKey = get(window, 'platform') === 'darwin' && metaKey; + const controlKey = get(window, 'platform') !== 'darwin' && ctrlKey; + const commandOrCtrl = commandKey || controlKey; + const commandAndCtrl = commandKey && ctrlKey; + + if (showingAvatarPopup && key === 'Escape') { + this.hideAvatarPopup(); + } else if ( + commandOrCtrl && + !commandAndCtrl && + !shiftKey && + (key === 'f' || key === 'F') + ) { + startSearch(); + + event.preventDefault(); + event.stopPropagation(); + } else if ( + selectedConversation && + commandOrCtrl && + !commandAndCtrl && + shiftKey && + (key === 'f' || key === 'F') + ) { + const name = selectedConversation.isMe + ? i18n('noteToSelf') + : selectedConversation.title; + searchInConversation(selectedConversation.id, name); + + event.preventDefault(); + event.stopPropagation(); + } + }; + public handleXButton = (): void => { const { searchConversationId, @@ -398,7 +442,7 @@ export class MainHeader extends React.Component { )} placeholder={placeholder} dir="auto" - onKeyDown={this.handleKeyDown} + onKeyDown={this.handleInputKeyDown} value={searchTerm} onChange={this.updateSearch} /> diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 3593df315..0c73476db 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -225,7 +225,7 @@ export type ConversationsStateType = { conversationsByE164: ConversationLookupType; conversationsByUuid: ConversationLookupType; conversationsByGroupId: ConversationLookupType; - selectedConversation?: string; + selectedConversationId?: string; selectedMessage?: string; selectedMessageCounter: number; selectedConversationTitle?: string; @@ -981,7 +981,7 @@ export function reducer( const { id, data } = payload; const { conversationLookup } = state; - let { showArchived, selectedConversation } = state; + let { showArchived, selectedConversationId } = state; const existing = conversationLookup[id]; // In the change case we only modify the lookup if we already had that conversation @@ -989,7 +989,7 @@ export function reducer( return state; } - if (selectedConversation === id) { + if (selectedConversationId === id) { // Archived -> Inbox: we go back to the normal inbox view if (existing.isArchived && !data.isArchived) { showArchived = false; @@ -999,13 +999,13 @@ export function reducer( // behavior - no selected conversation in the left pane, but a conversation show // in the right pane. if (!existing.isArchived && data.isArchived) { - selectedConversation = undefined; + selectedConversationId = undefined; } } return { ...state, - selectedConversation, + selectedConversationId, showArchived, conversationLookup: { ...conversationLookup, @@ -1040,14 +1040,14 @@ export function reducer( } const { messageIds } = existingConversation; - const selectedConversation = - state.selectedConversation !== id - ? state.selectedConversation + const selectedConversationId = + state.selectedConversationId !== id + ? state.selectedConversationId : undefined; return { ...state, - selectedConversation, + selectedConversationId, selectedConversationPanelDepth: 0, messagesLookup: omit(state.messagesLookup, messageIds), messagesByConversation: omit(state.messagesByConversation, [id]), @@ -1065,7 +1065,7 @@ export function reducer( if (action.type === 'MESSAGE_SELECTED') { const { messageId, conversationId } = action.payload; - if (state.selectedConversation !== conversationId) { + if (state.selectedConversationId !== conversationId) { return state; } @@ -1621,7 +1621,7 @@ export function reducer( return { ...state, - selectedConversation: id, + selectedConversationId: id, }; } if (action.type === 'SHOW_INBOX') { diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index 9cda1fbbe..079f7a9e9 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -22,6 +22,7 @@ import { getCallsByConversation } from './calling'; import { getBubbleProps } from '../../shims/Whisper'; import { PropsDataType as TimelinePropsType } from '../../components/conversation/Timeline'; import { TimelineItemType } from '../../components/conversation/TimelineItem'; +import { assert } from '../../util/assert'; import { getInteractionMode, @@ -83,10 +84,29 @@ export const getConversationsByGroupId = createSelector( } ); -export const getSelectedConversation = createSelector( +export const getSelectedConversationId = createSelector( getConversations, (state: ConversationsStateType): string | undefined => { - return state.selectedConversation; + return state.selectedConversationId; + } +); + +export const getSelectedConversation = createSelector( + getSelectedConversationId, + getConversationLookup, + ( + selectedConversationId: string | undefined, + conversationLookup: ConversationLookupType + ): undefined | ConversationType => { + if (!selectedConversationId) { + return undefined; + } + const conversation = getOwn(conversationLookup, selectedConversationId); + assert( + conversation, + 'getSelectedConversation: could not find selected conversation in lookup; returning undefined' + ); + return conversation; } ); @@ -221,7 +241,7 @@ export const _getLeftPaneLists = ( export const getLeftPaneLists = createSelector( getConversationLookup, getConversationComparator, - getSelectedConversation, + getSelectedConversationId, getPinnedConversationIds, _getLeftPaneLists ); diff --git a/ts/state/selectors/search.ts b/ts/state/selectors/search.ts index 069a126df..2bca7627d 100644 --- a/ts/state/selectors/search.ts +++ b/ts/state/selectors/search.ts @@ -29,7 +29,7 @@ import { GetConversationByIdType, getConversationLookup, getConversationSelector, - getSelectedConversation, + getSelectedConversationId, } from './conversations'; export const getSearch = (state: StateType): SearchStateType => state.search; @@ -78,7 +78,7 @@ export const getSearchResults = createSelector( getRegionCode, getUserAgent, getConversationLookup, - getSelectedConversation, + getSelectedConversationId, getSelectedMessage, ], ( diff --git a/ts/state/smart/LeftPane.tsx b/ts/state/smart/LeftPane.tsx index 4e0c6ea8e..839e1d8d9 100644 --- a/ts/state/smart/LeftPane.tsx +++ b/ts/state/smart/LeftPane.tsx @@ -11,7 +11,7 @@ import { getSearchResults, isSearching } from '../selectors/search'; import { getIntl } from '../selectors/user'; import { getLeftPaneLists, - getSelectedConversation, + getSelectedConversationId, getShowArchived, } from '../selectors/conversations'; @@ -52,7 +52,7 @@ const mapStateToProps = (state: StateType) => { const lists = showSearch ? undefined : getLeftPaneLists(state); const searchResults = showSearch ? getSearchResults(state) : undefined; - const selectedConversationId = getSelectedConversation(state); + const selectedConversationId = getSelectedConversationId(state); return { ...lists, diff --git a/ts/state/smart/MainHeader.tsx b/ts/state/smart/MainHeader.tsx index 958a1fc7e..1bc77fa88 100644 --- a/ts/state/smart/MainHeader.tsx +++ b/ts/state/smart/MainHeader.tsx @@ -20,13 +20,14 @@ import { getUserNumber, getUserUuid, } from '../selectors/user'; -import { getMe } from '../selectors/conversations'; +import { getMe, getSelectedConversation } from '../selectors/conversations'; const mapStateToProps = (state: StateType) => { return { searchTerm: getQuery(state), searchConversationId: getSearchConversationId(state), searchConversationName: getSearchConversationName(state), + selectedConversation: getSelectedConversation(state), startSearchCounter: getStartSearchCounter(state), regionCode: getRegionCode(state), ourConversationId: getUserConversationId(state), diff --git a/ts/test-both/state/selectors/conversations_test.ts b/ts/test-both/state/selectors/conversations_test.ts index b7f907721..6ba2a611b 100644 --- a/ts/test-both/state/selectors/conversations_test.ts +++ b/ts/test-both/state/selectors/conversations_test.ts @@ -13,6 +13,8 @@ import { _getLeftPaneLists, getConversationSelector, getPlaceholderContact, + getSelectedConversation, + getSelectedConversationId, } from '../../../state/selectors/conversations'; import { noopAction } from '../../../state/ducks/noop'; import { StateType, reducer as rootReducer } from '../../../state/reducer'; @@ -446,4 +448,65 @@ describe('both/state/selectors/conversations', () => { }); }); }); + + describe('#getSelectedConversationId', () => { + it('returns undefined if no conversation is selected', () => { + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + abc123: getDefaultConversation('abc123'), + }, + }, + }; + assert.isUndefined(getSelectedConversationId(state)); + }); + + it('returns the selected conversation ID', () => { + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + abc123: getDefaultConversation('abc123'), + }, + selectedConversationId: 'abc123', + }, + }; + assert.strictEqual(getSelectedConversationId(state), 'abc123'); + }); + }); + + describe('#getSelectedConversation', () => { + it('returns undefined if no conversation is selected', () => { + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + abc123: getDefaultConversation('abc123'), + }, + }, + }; + assert.isUndefined(getSelectedConversation(state)); + }); + + it('returns the selected conversation ID', () => { + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + abc123: getDefaultConversation('abc123'), + }, + selectedConversationId: 'abc123', + }, + }; + assert.deepEqual( + getSelectedConversation(state), + getDefaultConversation('abc123') + ); + }); + }); }); diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index bdcaaec26..e8ef749ba 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -14631,7 +14631,7 @@ "rule": "React-createRef", "path": "ts/components/MainHeader.js", "line": " this.inputRef = react_1.default.createRef();", - "lineNumber": 146, + "lineNumber": 171, "reasonCategory": "usageTrusted", "updated": "2020-02-14T20:02:37.507Z", "reasonDetail": "Used only to set focus" @@ -14640,7 +14640,7 @@ "rule": "React-createRef", "path": "ts/components/MainHeader.tsx", "line": " this.inputRef = React.createRef();", - "lineNumber": 74, + "lineNumber": 78, "reasonCategory": "usageTrusted", "updated": "2020-02-14T20:02:37.507Z", "reasonDetail": "Used only to set focus"