Skip to content

Commit

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

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix: [Obs Services > Create Service Group modal][KEYBOARD]: Focus
should not be auto-set on first input when modal appears
(#194696)](#194696)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alexey
Antonov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T12:04:39Z","message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Project:Accessibility","release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review"],"title":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal
appears","number":194696,"url":"https://github.com/elastic/kibana/pull/194696","mergeCommit":{"message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194696","number":194696,"mergeCommit":{"message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613"}}]}]
BACKPORT-->

Co-authored-by: Alexey Antonov <[email protected]>
  • Loading branch information
kibanamachine and alexwizp authored Oct 7, 2024
1 parent 337d938 commit 5ea090b
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 5ea090b

Please sign in to comment.