Skip to content

Commit

Permalink
fix: [Obs Services > Create Service Group modal][KEYBOARD]: Focus sho…
Browse files Browse the repository at this point in the history
…uld not be auto-set on first input when modal appears (elastic#194696)

Closes: elastic#194965
Closes: elastic#194966

# Description

- [x] elastic#194965 <br /> Focus is
currently being set on the first input in the "Create group" modal.
Screen reader users will hear the input name, but not get the title of
the modal read aloud this way, and it could be confusing. We should be
letting the EuiModal set focus naturally on the modal or close button so
screen reader users hear the title as expected.

- [x] elastic#194966 <br /> Focus must
be returned properly when I cancel the "Create group" workflow in
Services > Create service group modal.

# Changes Made

1. Removed:

```diff
- inputRef.current?.focus(); // autofocus on initial render
```

2. Added `aria-labelledby={modalTitleId}` for `EuiModal`. See
https://eui.elastic.co/#/layout/modal.
3. Slightly updated `Name` and `Color` validation.

# Screen

https://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7
(cherry picked from commit d576365)
  • Loading branch information
alexwizp committed Oct 7, 2024
1 parent 0776378 commit 7728bf2
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
isValidHex,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React, { useEffect, useRef, useState } from 'react';
import React, { useEffect, useState } from 'react';
import type { StagedServiceGroup } from './save_modal';

interface Props {
Expand All @@ -31,6 +31,7 @@ interface Props {
onClickNext: (serviceGroup: StagedServiceGroup) => void;
onDeleteGroup: () => void;
isLoading: boolean;
titleId?: string;
}

export function GroupDetails({
Expand All @@ -40,13 +41,16 @@ export function GroupDetails({
onClickNext,
onDeleteGroup,
isLoading,
titleId,
}: Props) {
const [name, setName] = useState<string>(serviceGroup?.groupName || '');
const [color, setColor, colorPickerErrors] = useColorPickerState(
serviceGroup?.color || '#5094C4'
);

const initialColor = serviceGroup?.color || '#5094C4';
const [name, setName] = useState(serviceGroup?.groupName);
const [color, setColor, colorPickerErrors] = useColorPickerState(initialColor);
const [description, setDescription] = useState<string | undefined>(serviceGroup?.description);

const isNamePristine = name === serviceGroup?.groupName;
const isColorPristine = color === initialColor;

useEffect(() => {
if (serviceGroup) {
setName(serviceGroup.groupName);
Expand All @@ -65,16 +69,10 @@ export function GroupDetails({
const isInvalidName = !name;
const isInvalid = isInvalidName || isInvalidColor;

const inputRef = useRef<HTMLInputElement | null>(null);

useEffect(() => {
inputRef.current?.focus(); // autofocus on initial render
}, []);

return (
<>
<EuiModalHeader>
<EuiModalHeaderTitle>
<EuiModalHeaderTitle id={titleId}>
{isEdit
? i18n.translate('xpack.apm.serviceGroups.groupDetailsForm.edit.title', {
defaultMessage: 'Edit group',
Expand All @@ -93,15 +91,25 @@ export function GroupDetails({
label={i18n.translate('xpack.apm.serviceGroups.groupDetailsForm.name', {
defaultMessage: 'Name',
})}
isInvalid={isInvalidName}
isInvalid={!isNamePristine && isInvalidName}
error={
!isNamePristine && isInvalidName
? i18n.translate(
'xpack.apm.serviceGroups.groupDetailsForm.invalidNameError',
{
defaultMessage: 'Please provide a valid name value',
}
)
: undefined
}
>
<EuiFieldText
data-test-subj="apmGroupNameInput"
value={name}
value={name || ''}
onChange={(e) => {
setName(e.target.value);
}}
inputRef={inputRef}
isInvalid={!isNamePristine && isInvalidName}
/>
</EuiFormRow>
</EuiFlexItem>
Expand All @@ -110,9 +118,9 @@ export function GroupDetails({
label={i18n.translate('xpack.apm.serviceGroups.groupDetailsForm.color', {
defaultMessage: 'Color',
})}
isInvalid={isInvalidColor}
isInvalid={!isColorPristine && isInvalidColor}
error={
isInvalidColor
!isColorPristine && isInvalidColor
? i18n.translate(
'xpack.apm.serviceGroups.groupDetailsForm.invalidColorError',
{
Expand All @@ -122,7 +130,11 @@ export function GroupDetails({
: undefined
}
>
<EuiColorPicker onChange={setColor} color={color} isInvalid={isInvalidColor} />
<EuiColorPicker
onChange={setColor}
color={color}
isInvalid={!isColorPristine && isInvalidColor}
/>
</EuiFormRow>
</EuiFlexItem>
</EuiFlexGroup>
Expand All @@ -144,7 +156,7 @@ export function GroupDetails({
<EuiFieldText
data-test-subj="apmGroupDetailsFieldText"
fullWidth
value={description}
value={description || ''}
onChange={(e) => {
setDescription(e.target.value);
}}
Expand All @@ -164,6 +176,7 @@ export function GroupDetails({
onDeleteGroup();
}}
color="danger"
isLoading={isLoading}
isDisabled={isLoading}
data-test-subj="apmDeleteGroupButton"
>
Expand All @@ -177,6 +190,7 @@ export function GroupDetails({
<EuiButtonEmpty
data-test-subj="apmGroupDetailsCancelButton"
onClick={onCloseModal}
isLoading={isLoading}
isDisabled={isLoading}
>
{i18n.translate('xpack.apm.serviceGroups.groupDetailsForm.cancel', {
Expand All @@ -192,12 +206,13 @@ export function GroupDetails({
iconSide="right"
onClick={() => {
onClickNext({
groupName: name,
groupName: name || '',
color,
description,
kuery: serviceGroup?.kuery ?? '',
});
}}
isLoading={isLoading}
isDisabled={isInvalid || isLoading}
>
{i18n.translate('xpack.apm.serviceGroups.groupDetailsForm.selectServices', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { EuiModal } from '@elastic/eui';
import { EuiModal, useGeneratedHtmlId } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { useHistory } from 'react-router-dom';
import React, { useCallback, useEffect, useState } from 'react';
Expand Down Expand Up @@ -89,6 +89,8 @@ export function SaveGroupModal({ onClose, savedServiceGroup }: Props) {
[savedServiceGroup?.id, notifications.toasts, onClose, isEdit, navigateToServiceGroups]
);

const modalTitleId = useGeneratedHtmlId();

const onDelete = useCallback(
async function () {
setIsLoading(true);
Expand All @@ -115,7 +117,7 @@ export function SaveGroupModal({ onClose, savedServiceGroup }: Props) {
);

return (
<EuiModal onClose={onClose}>
<EuiModal onClose={onClose} aria-labelledby={modalTitleId}>
{modalView === 'group_details' && (
<GroupDetails
serviceGroup={stagedServiceGroup}
Expand All @@ -127,6 +129,7 @@ export function SaveGroupModal({ onClose, savedServiceGroup }: Props) {
}}
onDeleteGroup={onDelete}
isLoading={isLoading}
titleId={modalTitleId}
/>
)}
{modalView === 'select_service' && stagedServiceGroup && (
Expand All @@ -139,6 +142,7 @@ export function SaveGroupModal({ onClose, savedServiceGroup }: Props) {
setModalView('group_details');
}}
isLoading={isLoading}
titleId={modalTitleId}
/>
)}
</EuiModal>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ interface Props {
onSaveClick: (serviceGroup: StagedServiceGroup) => void;
onEditGroupDetailsClick: () => void;
isLoading: boolean;
titleId?: string;
}

export function SelectServices({
Expand All @@ -63,6 +64,7 @@ export function SelectServices({
onSaveClick,
onEditGroupDetailsClick,
isLoading,
titleId,
}: Props) {
const [kuery, setKuery] = useState(serviceGroup?.kuery || '');
const [stagedKuery, setStagedKuery] = useState(serviceGroup?.kuery || '');
Expand Down Expand Up @@ -117,7 +119,7 @@ export function SelectServices({
<Container>
<EuiModalHeader>
<div>
<EuiModalHeaderTitle>
<EuiModalHeaderTitle id={titleId}>
{i18n.translate('xpack.apm.serviceGroups.selectServicesForm.title', {
defaultMessage: 'Select services',
})}
Expand Down

0 comments on commit 7728bf2

Please sign in to comment.