[Research] OUI Compliance audit of the saved_objects plugin

[sayuree]

Audit

The saved_objects plugin contains 3 Sass files: index.scss, saved_object_save_modal.scss and _index.scss. index.scss and _index.scss are importing a style from saved_object_save_modal.scss, which has a single class:

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.scss

Lines 1 to 3 in bd7d707

	.osdSavedObjectSaveModal {
	width: $euiSizeXXL * 10;
	}

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/_index.scss

Line 1 in bd7d707

@import "./saved_object_save_modal";

OpenSearch-Dashboards/src/plugins/saved_objects/public/index.scss

Line 1 in bd7d707

@import "./save_modal/index";

Related React code:

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Lines 102 to 106 in bd7d707

	<EuiModal
	data-test-subj="savedObjectSaveModal"
	className="osdSavedObjectSaveModal"
	onClose={this.props.onClose}
	>

There are no img, span, but div, p HTML tags are present, outnumbered by OUI library components.
When a specific value was needed, OUI variable was used: euiSizeXXL
The custom components are predominantly made of OUI library components and are used correctly:

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.test.tsx

Lines 40 to 47 in bd7d707

	<SavedObjectSaveModal
	onSave={() => void 0}
	onClose={() => void 0}
	title={'Saved Object title'}
	showCopyOnSave={false}
	objectType="visualization"
	showDescription={true}
	/>

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.test.tsx

Lines 54 to 62 in bd7d707

	<SavedObjectSaveModal
	onSave={() => void 0}
	onClose={() => void 0}
	title={'Saved Object title'}
	showCopyOnSave={false}
	objectType="visualization"
	showDescription={true}
	confirmButtonLabel="Save and done"
	/>

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Lines 102 to 166 in bd7d707

	<EuiModal
	data-test-subj="savedObjectSaveModal"
	className="osdSavedObjectSaveModal"
	onClose={this.props.onClose}
	>
	<EuiModalHeader>
	<EuiModalHeaderTitle>
	<FormattedMessage
	id="savedObjects.saveModal.saveTitle"
	defaultMessage="Save {objectType}"
	values={{ objectType: this.props.objectType }}
	/>
	</EuiModalHeaderTitle>
	</EuiModalHeader>
	<EuiModalBody>
	{this.renderDuplicateTitleCallout(duplicateWarningId)}
	<EuiForm component="form" onSubmit={this.onFormSubmit} id="savedObjectSaveModalForm">
	{!this.props.showDescription && this.props.description && (
	<EuiText size="s" color="subdued">
	{this.props.description}
	</EuiText>
	)}
	<EuiSpacer />
	{this.renderCopyOnSave()}
	<EuiFormRow
	fullWidth
	label={
	<FormattedMessage id="savedObjects.saveModal.titleLabel" defaultMessage="Title" />
	}
	>
	<EuiFieldText
	fullWidth
	autoFocus
	data-test-subj="savedObjectTitle"
	value={title}
	onChange={this.onTitleChange}
	isInvalid={(!isTitleDuplicateConfirmed && hasTitleDuplicate) || title.length === 0}
	aria-describedby={this.state.hasTitleDuplicate ? duplicateWarningId : undefined}
	/>
	</EuiFormRow>
	{this.renderViewDescription()}
	{typeof this.props.options === 'function'
	? this.props.options(this.state)
	: this.props.options}
	</EuiForm>
	</EuiModalBody>
	<EuiModalFooter>
	<EuiButtonEmpty data-test-subj="saveCancelButton" onClick={this.props.onClose}>
	<FormattedMessage
	id="savedObjects.saveModal.cancelButtonLabel"
	defaultMessage="Cancel"
	/>
	</EuiButtonEmpty>
	{this.renderConfirmButton()}
	</EuiModalFooter>
	</EuiModal>

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Lines 265 to 274 in bd7d707

	<EuiButton
	fill
	data-test-subj="confirmSaveSavedObjectButton"
	isLoading={isLoading}
	isDisabled={title.length === 0}
	type="submit"
	form="savedObjectSaveModalForm"
	>
	{confirmLabel}
	</EuiButton>

However, HTML tags are also present in the following places:

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Lines 284 to 310 in bd7d707

	<>
	<div ref={this.warning} tabIndex={-1}>
	<EuiCallOut
	title={
	<FormattedMessage
	id="savedObjects.saveModal.duplicateTitleLabel"
	defaultMessage="This {objectType} already exists"
	values={{ objectType: this.props.objectType }}
	/>
	}
	color="warning"
	data-test-subj="titleDupicateWarnMsg"
	id={duplicateWarningId}
	>
	<p>
	<FormattedMessage
	id="savedObjects.saveModal.duplicateTitleDescription"
	defaultMessage="Saving '{title}' creates a duplicate title."
	values={{
	title: this.state.title,
	}}
	/>
	</p>
	</EuiCallOut>
	</div>
	<EuiSpacer />
	</>

For the focus management, we have ref and tabindex defined in div tag. I assume we could use EuiPanel with the property panelRef of type Ref<HTMLDivElement> and extends HTMLElement, allowing us to define tabindex property as well.

OpenSearch-Dashboards/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Lines 320 to 334 in bd7d707

	<>
	<EuiSwitch
	data-test-subj="saveAsNewCheckbox"
	checked={this.state.copyOnSave}
	onChange={this.onCopyOnSaveChange}
	label={
	<FormattedMessage
	id="savedObjects.saveModal.saveAsNewLabel"
	defaultMessage="Save as new {objectType}"
	values={{ objectType: this.props.objectType }}
	/>
	}
	/>
	<EuiSpacer />
	</>

FormattedMessage is wrapped into p tag for customized rendering (recommended practice):

By default will render the formatted string into a <React.Fragment>. If you need to customize rendering, you can either wrap it with another React element (recommended), specify a different tagName (e.g., 'div'), or pass a function as the child.

Conclusion

1. OUI library components are mainly used throughout the saved_objects plugin, there are no issues with the usage;
2. Replace tags used for wrapping other components with EuiPanel, including cases related to focus management;
3. No need to remove p tag, enveloping FormattedMessage;

[joshuarrrr]

@sayuree Thanks for the audit, there's lots of useful analysis here.

...importing a style from saved_object_save_modal.scss, which has a single class:

The first goal of these audits is to simply identify the styles and components, as you've done here. However, when analyzing and recommending mitigations, the goal is to fully remove any custom styles, such as this one. To do so, we need to try to figure out 1) what the style is doing, and 2) how we might solve the same problem, or achieve similar outcomes with a more compliant approach.

In this case, 1) is fairly obvious: it's setting a static width of the modal. But why? The most likely reasons are either to specify a different minimum or maximum modal width. Each of those scenarios is already accounted for in OUI, without the need for a custom class and style: https://oui.opensearch.org/1.0/#/layout/modal

To help UX assess whether a custom width is necessary at all, can you provide a screenshot of this modal and instructions on how to navigate/activate it on https://playground.opensearch.org ?