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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Sep 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0ac5b98
Convert ES body of discover doc request to use fields API
kertal Aug 19, 2020
2a091d3
Use field.name instead of idx for key in li element
kertal Aug 24, 2020
2ef14f6
Undo accidentally commited change
kertal Aug 24, 2020
3b118c6
onClick add focus to button
kertal Aug 27, 2020
12acf3a
Merge branch 'master' into kertal-pr-2020-08-24-fix-discover-sidebar-…
elasticmachine Aug 27, 2020
7994764
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-08…
kertal Aug 31, 2020
88b6e76
Prevent Chrome jumping back to last focused element
kertal Aug 31, 2020
4f26d30
Merge branch 'kertal-pr-2020-08-24-fix-discover-sidebar-focus-behavio…
kertal Aug 31, 2020
8fe749e
Merge branch 'master' into kertal-pr-2020-08-24-fix-discover-sidebar-…
elasticmachine Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/plugins/discover/public/application/angular/discover.html
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions src/plugins/discover/public/application/components/sidebar/discover_field.tsx
View file
Open in desktop
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
22 changes: 10 additions & 12 deletions src/plugins/discover/public/application/components/sidebar/discover_sidebar.tsx
View file
Open in desktop
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
1 change: 0 additions & 1 deletion src/plugins/discover/public/application/components/sidebar/discover_sidebar_directive.ts
View file
Open in desktop
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
View file
Open in desktop
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