fix(preview): Close color palette when tabbed out of the palette focus #1435
Conversation
|
|
| const { current: paletteEl } = paletteRef; | ||
|
|
||
| const handlePaletteBlur = ({ relatedTarget }: React.FocusEvent<HTMLButtonElement>): void => { | ||
| const targetElement = relatedTarget as HTMLButtonElement; |
There was a problem hiding this comment.
I don't think IE11 supports relatedTarget, so we may need to fall back to document.activeElement here as in the handleToggleBlur function below.
|
|
||
| const handlePaletteBlur = ({ relatedTarget }: React.FocusEvent<HTMLButtonElement>): void => { | ||
| const targetElement = relatedTarget as HTMLButtonElement; | ||
| if (targetElement.parentElement && targetElement.parentElement.parentElement !== paletteEl) { |
There was a problem hiding this comment.
The parentElement calls here make some assumptions about the DOM that may eventually change and break unexpectedly. Can we use !paletteEl.contains(targetElement), instead?
| } | ||
| }; | ||
|
|
||
| const [isPaletteActive, handlers] = useAttention({ onBlur: handlePaletteBlur }); |
There was a problem hiding this comment.
Can we add onBlur to <ColorPickerPalette> to avoid having to modify useAttention with separate callbacks?
|
|
||
| const handleSelect = (color: string): void => { | ||
| setIsColorPickerToggled(false); | ||
| onColorSelect(color); | ||
| }; | ||
|
|
||
| const handleBlur = ({ relatedTarget }: React.FocusEvent<HTMLButtonElement>): void => { | ||
| const { current: paletteEl } = paletteRef; | ||
| const handleToggleBlur = ({ relatedTarget }: React.FocusEvent<HTMLButtonElement>): void => { |
There was a problem hiding this comment.
Looking at it again, it seems like both of the blur handlers are doing the same thing, just in different ways. Can we use the same callback for both the toggle and the palette?
There was a problem hiding this comment.
Yeah just realized I can use the same handler if I take out the isPaletteActive conditional. I didn't see any focus issues in taking that conditional out.
| const [isColorPickerToggled, setIsColorPickerToggled] = React.useState(false); | ||
| const [isPaletteActive, handlers] = useAttention(); | ||
| const [handlers] = useAttention(); | ||
| const { current: paletteEl } = paletteRef; |
There was a problem hiding this comment.
Can we revert this change? We typically destructure these inside the event handler where they're used, since the value could theoretically change before they're invoked.
| // IE11 does not have relatedTarget but update activeElement before blur | ||
| const nextTarget = relatedTarget || document.activeElement; | ||
|
|
||
| if (isPaletteActive || (nextTarget && paletteEl && paletteEl.contains(nextTarget as Node))) { | ||
| if ((nextTarget && paletteEl && paletteEl.contains(nextTarget as Node)) || toggleEl === (nextTarget as Node)) { |
There was a problem hiding this comment.
Can we make this a bit more readable by using some additional variables?
const { current: paletteEl } = paletteRef;
const { current: toggleEl } = toggleRef;
// IE11 does not have relatedTarget but update activeElement before blur
const nextTarget = (relatedTarget || document.activeElement) as Node;
const isNextInPalette = paletteEl && paletteEl.contains(nextTarget);
const isNextToggle = toggleEl && toggleEl === nextTarget;
if (isNextInPalette || isNextToggle) {
return;
}
setIsColorPickerToggled(false);
| return; | ||
| } | ||
|
|
||
| setIsColorPickerToggled(false); | ||
| }; | ||
|
|
||
| const handleClick = (): void => setIsColorPickerToggled(!isColorPickerToggled); | ||
| const handleClick = (): void => { |
There was a problem hiding this comment.
Is this change needed or can we revert it?
There was a problem hiding this comment.
forgot that, thanks
| onSelect: (color: string) => void; | ||
| }; | ||
|
|
||
| export default function ColorPickerPalette({ colors, onSelect }: Props): JSX.Element { | ||
| export default function ColorPickerPalette({ colors, onSelect, onBlur }: Props): JSX.Element { |
There was a problem hiding this comment.
We typically alphabetize our props, attributes, etc. when possible.
| test('should close the palette when focus is outside palette', () => { | ||
| const wrapper = getWrapper(); | ||
| const toggleButton = getToggleButton(wrapper); | ||
| const divNode = document.createElement('div'); |
There was a problem hiding this comment.
We would typically refer to this as divEl or divElement, since we know it's an Element object rather than a generic Node.
| toggleButton.simulate('click'); | ||
| expect(getColorPickerPalette(wrapper).hasClass('bp-is-open')).toBe(true); | ||
|
|
||
| toggleButton.simulate('blur', { |
There was a problem hiding this comment.
Should we be testing the palette instead of the button? Do we also need a test for when the nextTarget after blur is the toggle button?
| <ColorPickerPalette colors={colors} data-testid="bp-ColorPickerPalette" onSelect={handleSelect} /> | ||
| <ColorPickerPalette | ||
| colors={colors} | ||
| data-testid="bp-ColorPickerPalette" |
There was a problem hiding this comment.
This isn't related to your change, but does this get applied to the palette? It's not a listed prop and we don't have a ...rest operator anywhere. Can we remove it safely?
| const [isColorPickerToggled, setIsColorPickerToggled] = React.useState(false); | ||
| const [isPaletteActive, handlers] = useAttention(); | ||
| const [handlers] = useAttention(); |
There was a problem hiding this comment.
Do we still need useAttention if we're not using the state it returns?
There was a problem hiding this comment.
Good point, will remove
JChan106
force-pushed
the
fix-palette-focus
branch
from
e507e84 to
0b082a0
Compare
JChan106
force-pushed
the
fix-palette-focus
branch
from
0b082a0 to
887d54a
Compare
Fixes the bug where focusing out of the color palette using keyboard navigation didn't close the palette. The palette now closes when a user tabs out of the color palette, or if the user is tabbed in the palette and the user clicks out of the palette.