Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

9 changes: 4 additions & 5 deletions src/plugins/discover/public/application/angular/discover.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ <h1 class="euiScreenReaderOnly">{{screenTitle}}</h1>
on-remove-field="removeColumn"
selected-index-pattern="searchSource.getField('index')"
set-index-pattern="setIndexPattern"
state="state"
>
</discover-sidebar>
</div>
Expand Down Expand Up @@ -78,11 +77,11 @@ <h1 class="euiScreenReaderOnly">{{screenTitle}}</h1>
class="dscTimechart"
ng-if="opts.timefield"
>
<timechart-header
<timechart-header
from="toMoment(timeRange.from)"
to="toMoment(timeRange.to)"
options="intervalOptions"
on-change-interval="changeInterval"
to="toMoment(timeRange.to)"
options="intervalOptions"
on-change-interval="changeInterval"
state-interval="state.interval"
show-scaled-info="bucketInterval.scaled"
bucket-interval-description="bucketInterval.description"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ export function DiscoverField({
iconType="plusInCircleFilled"
className="dscSidebarItem__action"
onClick={(ev: React.MouseEvent<HTMLButtonElement>) => {
if (ev.type === 'click') {
ev.currentTarget.focus();
}
Comment on lines +136 to +138
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreadelrio this should prevent Chrome from setting focus to the last focused sidebar element in case of a click

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this and it seems to solve the issue properly.

ev.preventDefault();
ev.stopPropagation();
toggleDisplay(field);
Expand All @@ -155,6 +158,9 @@ export function DiscoverField({
iconType="cross"
className="dscSidebarItem__action"
onClick={(ev: React.MouseEvent<HTMLButtonElement>) => {
if (ev.type === 'click') {
ev.currentTarget.focus();
}
ev.preventDefault();
ev.stopPropagation();
toggleDisplay(field);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import { SavedObject } from '../../../../../../core/types';
import { FIELDS_LIMIT_SETTING } from '../../../../common';
import { groupFields } from './lib/group_fields';
import { IndexPatternField, IndexPattern, UI_SETTINGS } from '../../../../../data/public';
import { AppState } from '../../angular/discover_state';
import { getDetails } from './lib/get_details';
import { getDefaultFieldFilter, setFieldFilterProp } from './lib/field_filter';
import { getIndexPatternFieldList } from './lib/get_index_pattern_field_list';
Expand Down Expand Up @@ -74,10 +73,6 @@ export interface DiscoverSidebarProps {
* Callback function to select another index pattern
*/
setIndexPattern: (id: string) => void;
/**
* Current app state, used for generating a link to visualize
*/
state: AppState;
}

export function DiscoverSidebar({
Expand All @@ -90,7 +85,6 @@ export function DiscoverSidebar({
onRemoveField,
selectedIndexPattern,
setIndexPattern,
state,
}: DiscoverSidebarProps) {
const [showFields, setShowFields] = useState(false);
const [fields, setFields] = useState<IndexPatternField[] | null>(null);
Expand Down Expand Up @@ -185,10 +179,10 @@ export function DiscoverSidebar({
aria-labelledby="selected_fields"
data-test-subj={`fieldList-selected`}
>
{selectedFields.map((field: IndexPatternField, idx: number) => {
{selectedFields.map((field: IndexPatternField) => {
return (
<li
key={`field${idx}`}
key={`field${field.name}`}
data-attr-field={field.name}
className="dscSidebar__item"
>
Expand Down Expand Up @@ -260,10 +254,10 @@ export function DiscoverSidebar({
aria-labelledby="available_fields available_fields_popular"
data-test-subj={`fieldList-popular`}
>
{popularFields.map((field: IndexPatternField, idx: number) => {
{popularFields.map((field: IndexPatternField) => {
return (
<li
key={`field${idx}`}
key={`field${field.name}`}
data-attr-field={field.name}
className="dscSidebar__item"
>
Expand All @@ -290,9 +284,13 @@ export function DiscoverSidebar({
aria-labelledby="available_fields"
data-test-subj={`fieldList-unpopular`}
>
{unpopularFields.map((field: IndexPatternField, idx: number) => {
{unpopularFields.map((field: IndexPatternField) => {
return (
<li key={`field${idx}`} data-attr-field={field.name} className="dscSidebar__item">
<li
key={`field${field.name}`}
data-attr-field={field.name}
className="dscSidebar__item"
>
<DiscoverField
field={field}
indexPattern={selectedIndexPattern}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,5 @@ export function createDiscoverSidebarDirective(reactDirective: any) {
['onRemoveField', { watchDepth: 'reference' }],
['selectedIndexPattern', { watchDepth: 'reference' }],
['setIndexPattern', { watchDepth: 'reference' }],
['state', { watchDepth: 'reference' }],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR also contains a few cleanups of unused state variable

]);
}
11 changes: 10 additions & 1 deletion src/plugins/kibana_react/public/field_button/field_button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,16 @@ export function FieldButton({

return (
<div className={classes} {...rest}>
<button onClick={onClick} {...buttonProps} className={buttonClasses}>
<button
onClick={(e) => {
if (e.type === 'click') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, if it is ever something else than 'click'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it could also be triggered by an enter key, but in this case it should already have focus

e.currentTarget.focus();
}
onClick();
}}
kertal marked this conversation as resolved.
Show resolved Hide resolved
{...buttonProps}
className={buttonClasses}
>
{fieldIcon && <span className="kbnFieldButton__fieldIcon">{fieldIcon}</span>}
{fieldName && <span className="kbnFieldButton__name">{fieldName}</span>}
{fieldInfoIcon && <div className="kbnFieldButton__infoIcon">{fieldInfoIcon}</div>}
Expand Down