Skip to content

Commit a55308d

Browse files
mattrunyonmattrunyon
authored
fix: DH-12163 - Column grouping sidebar test failure fixes (#1142)
- Does not select new groups when searching - Shows validation error if you create a new group and blur without typing anything into the name field - Prevents Firefox on Mac from getting stuck in a bad state where color pickers can't be opened. The bug occurs when multiple color inputs are on the screen at once and the user clicks between the without closing the previous picker
1 parent 2fbccc6 commit a55308d

3 files changed

Lines changed: 126 additions & 47 deletions

File tree

‎packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.test.tsx‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -844,11 +844,49 @@ test('Only allows 1 new group at a time', async () => {
844844
expect(mockGroupHandler).toBeCalledWith([
845845
expect.objectContaining(groupObject),
846846
]);
847+
848+
createGroupBtn.focus();
849+
expect(screen.queryAllByText('Invalid name').length).toBe(1);
850+
});
851+
852+
test('Shows validation error for new group on blur when never typed in', async () => {
853+
const user = userEvent.setup({ delay: null });
854+
855+
const model = makeModelWithGroups([
856+
{
857+
children: [`${COLUMN_PREFIX}0`, `${COLUMN_PREFIX}1`],
858+
name: `${ColumnHeaderGroup.NEW_GROUP_PREFIX}Test`,
859+
},
860+
]);
861+
862+
const mockGroupHandler = jest.fn();
863+
864+
render(
865+
<Builder
866+
model={model}
867+
columnHeaderGroups={model.columnHeaderGroups}
868+
onColumnHeaderGroupChanged={mockGroupHandler}
869+
/>
870+
);
871+
872+
expect(screen.queryAllByText('Invalid name').length).toBe(0);
873+
874+
await selectItems(user, [2]);
875+
expect(screen.queryAllByText('Invalid name').length).toBe(1);
847876
});
848877

849878
test('Search columns', async () => {
850879
const user = userEvent.setup({ delay: null });
851-
render(<BuilderWithGroups />);
880+
881+
const model = makeModelWithGroups([
882+
...COLUMN_HEADER_GROUPS,
883+
{
884+
name: `${ColumnHeaderGroup.NEW_GROUP_PREFIX}Test`,
885+
children: [COLUMNS[9].name],
886+
},
887+
]);
888+
889+
render(<BuilderWithGroups model={model} />);
852890

853891
const searchInput = screen.getByPlaceholderText('Search');
854892

@@ -861,19 +899,20 @@ test('Search columns', async () => {
861899
expectSelection([1, 2, 3, 4, 5, 6]);
862900

863901
await user.type(searchInput, 'One');
864-
865902
jest.advanceTimersByTime(500);
866-
867903
expectSelection([1, 2, 3]);
868904

869905
await user.clear(searchInput);
870-
871906
jest.advanceTimersByTime(500);
872-
873907
expectSelection([]);
874908

875909
await user.type(searchInput, 'asdf');
910+
jest.advanceTimersByTime(500);
911+
expectSelection([]);
876912

913+
await user.clear(searchInput);
914+
jest.advanceTimersByTime(500);
915+
await user.type(searchInput, ColumnHeaderGroup.NEW_GROUP_PREFIX);
877916
jest.advanceTimersByTime(500);
878917
expectSelection([]);
879918
});

‎packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.tsx‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,10 @@ class VisibilityOrderingBuilder extends Component<
151151

152152
searchColumns(searchFilter: string): void {
153153
const flattenedItems = flattenTree(this.getTreeItems());
154-
const itemsMatch = flattenedItems.filter(({ id }) =>
155-
id.toLowerCase().includes(searchFilter.toLowerCase())
154+
const itemsMatch = flattenedItems.filter(
155+
({ id, data }) =>
156+
!(data.group?.isNew ?? false) &&
157+
id.toLowerCase().includes(searchFilter.toLowerCase())
156158
);
157159

158160
const columnsMatch = itemsMatch.map(({ id }) => id);

‎packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingGroup.tsx‎

Lines changed: 78 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { useEffect, useRef, useState } from 'react';
1+
import React, { useEffect, useRef, useState, useCallback } from 'react';
22
import classNames from 'classnames';
33
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
44
import { Button, ThemeExport } from '@deephaven/components';
@@ -29,11 +29,15 @@ export default function VisibilityOrderingGroup(
2929
const groupRef = useRef(group);
3030
const nameInputRef = useRef<HTMLInputElement>(null);
3131
const colorInputRef = useRef<HTMLInputElement>(null);
32+
const [isColorInputOpen, setIsColorInputOpen] = useState(false);
3233
const [name, setName] = useState(isNew ? '' : group.name);
3334
const [isEditing, setIsEditing] = useState(isNew);
34-
const [hasTyped, setHasTyped] = useState(false);
35+
const [shouldValidate, setShouldValidate] = useState(false);
3536
const nameValidationError = name !== group.name ? validateName(name) : '';
36-
const isValid = (isNew && !hasTyped) || nameValidationError === '';
37+
const isValid = (isNew && !shouldValidate) || nameValidationError === '';
38+
const colorInputBlurHandler = useCallback(() => {
39+
setIsColorInputOpen(false);
40+
}, []);
3741

3842
useEffect(
3943
function focusEditInput() {
@@ -59,6 +63,35 @@ export default function VisibilityOrderingGroup(
5963
[onDelete]
6064
);
6165

66+
useEffect(
67+
function openColorInput() {
68+
if (isColorInputOpen) {
69+
colorInputRef.current?.click();
70+
// Mostly for testing. Chrome seems to not give the hidden input focus
71+
// Really would only affect screen readers
72+
colorInputRef.current?.focus();
73+
74+
/**
75+
* Adding this event handler is for Firefox on Mac
76+
* There seems to be buggy behavior when multiple color inputs are on the same page
77+
* Clicking between the inputs without closing the previous causes a bad state
78+
* The user gets to a point where they can't open most of the pickers
79+
* https://bugzilla.mozilla.org/show_bug.cgi?id=1618418
80+
* https://bugzilla.mozilla.org/show_bug.cgi?id=975468
81+
* Instead, we remove the color input when any focus is returned to the window
82+
* This causes Firefox on Mac to mostly operate correctly
83+
* Firefox seems to ignore the first click back into the window and emit no event
84+
* So opening a color picker when another is open requires 2 clicks in Firefox
85+
*/
86+
window.addEventListener('click', colorInputBlurHandler, true);
87+
}
88+
89+
return () =>
90+
window.removeEventListener('click', colorInputBlurHandler, true);
91+
},
92+
[isColorInputOpen, colorInputBlurHandler]
93+
);
94+
6295
const handleConfirm = () => {
6396
if (isValid) {
6497
onNameChange(group, name);
@@ -76,7 +109,7 @@ export default function VisibilityOrderingGroup(
76109
};
77110

78111
const handleEditKeyDown = (e: React.KeyboardEvent): void => {
79-
setHasTyped(true);
112+
setShouldValidate(true);
80113
if (e.key === 'Enter') {
81114
e.stopPropagation();
82115
handleConfirm();
@@ -104,6 +137,7 @@ export default function VisibilityOrderingGroup(
104137
placeholder="Group Name"
105138
onChange={e => setName(e.target.value)}
106139
onKeyDown={handleEditKeyDown}
140+
onBlur={() => setShouldValidate(true)}
107141
/>
108142
<Button
109143
kind="ghost"
@@ -171,43 +205,47 @@ export default function VisibilityOrderingGroup(
171205
)
172206
}
173207
tooltip="Set color"
174-
onClick={() => {
175-
colorInputRef.current?.click();
176-
// Mostly for testing. Chrome seems to not give the hidden input focus
177-
// Really would only affect screen readers
178-
colorInputRef.current?.focus();
179-
}}
208+
/**
209+
* Toggle to close the picker on Chrome
210+
* Prevents Firefox on Mac from getting into a stuck state
211+
* Does not close on Firefox b/c the picker stays open when the element is removed
212+
*/
213+
onClick={() => setIsColorInputOpen(val => !val)}
180214
/>
181-
<input
182-
aria-label="Color input"
183-
ref={colorInputRef}
184-
type="color"
185-
list="presetColors"
186-
value={group.color ?? ThemeExport['content-bg']}
187-
style={{
188-
visibility: 'hidden',
189-
width: 0,
190-
height: 0,
191-
padding: 0,
192-
border: 0,
193-
}}
194-
onChange={e => {
195-
group.color = e.target.value;
196-
onColorChange(group, e.target.value);
197-
}}
198-
/>
199-
<datalist id="presetColors">
200-
<option>{ThemeExport['content-bg']}</option>
201-
<option>{ThemeExport.primary}</option>
202-
<option>{ThemeExport.foreground}</option>
203-
<option>{ThemeExport.green}</option>
204-
<option>{ThemeExport.yellow}</option>
205-
<option>{ThemeExport.orange}</option>
206-
<option>{ThemeExport.red}</option>
207-
<option>{ThemeExport.purple}</option>
208-
<option>{ThemeExport.blue}</option>
209-
<option>{ThemeExport['gray-400']}</option>
210-
</datalist>
215+
{isColorInputOpen && (
216+
<>
217+
<input
218+
aria-label="Color input"
219+
ref={colorInputRef}
220+
type="color"
221+
list="presetColors"
222+
value={group.color ?? ThemeExport['content-bg']}
223+
style={{
224+
visibility: 'hidden',
225+
width: 0,
226+
height: 0,
227+
padding: 0,
228+
border: 0,
229+
}}
230+
onChange={e => {
231+
group.color = e.target.value;
232+
onColorChange(group, e.target.value);
233+
}}
234+
/>
235+
<datalist id="presetColors">
236+
<option>{ThemeExport['content-bg']}</option>
237+
<option>{ThemeExport.primary}</option>
238+
<option>{ThemeExport.foreground}</option>
239+
<option>{ThemeExport.green}</option>
240+
<option>{ThemeExport.yellow}</option>
241+
<option>{ThemeExport.orange}</option>
242+
<option>{ThemeExport.red}</option>
243+
<option>{ThemeExport.purple}</option>
244+
<option>{ThemeExport.blue}</option>
245+
<option>{ThemeExport['gray-400']}</option>
246+
</datalist>
247+
</>
248+
)}
211249
</span>
212250
</div>
213251
);

0 commit comments

Comments
 (0)
⚡