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

Fix accessibility issues with saved object finder #13152

Merged
merged 6 commits into from
Aug 7, 2017
Merged

Fix accessibility issues with saved object finder #13152

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bbc77cb
Fix accessibility issues with saved object finder
stacey-gammon Jul 27, 2017
d0d3972
Dark theme-icy kuiTabs
stacey-gammon Jul 28, 2017
6ac6015
Refer to existing dark theme color variables. Use dark theme hover l…
stacey-gammon Jul 31, 2017
ae0325b
use button instead of div element so no need for kbn-accessible-click
stacey-gammon Jul 31, 2017
98147ae
Add dark theme tab variety to ui framework site, lighten color of bac…
stacey-gammon Aug 2, 2017
5295896
Merge branch 'master' of https://github.com/elastic/kibana into fix/a…
stacey-gammon Aug 2, 2017
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
38 changes: 20 additions & 18 deletions src/core_plugins/kibana/public/dashboard/top_nav/add_panel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,26 @@
>
<div class="kuiLocalDropdownTitle">Add Panels</div>

<ul class="nav nav-tabs">
<li ng-class="{active: mode == 'visualization'}">
<a
ng-click="mode='visualization'"
aria-label="List visualizations"
>
Visualization
</a>
</li>
<li ng-class="{active: mode == 'search'}">
<a
ng-click="mode='search'"
aria-label="List saved searches"
>
Saved Search
</a>
</li>
</ul>
<div class="kuiTabs">
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be <button> elements, and then we won't need kbn-accessible-click any more. That directive is meant to be used as a last resort, if you can't use a <button> or <a> with an href.

kbn-accessible-click
ng-class="{ 'kuiTab-isSelected': mode == 'visualization'}"
class="kuiTab"
ng-click="mode='visualization'"
aria-label="List visualizations"
>
Visualization
</div>
<div
kbn-accessible-click
ng-class="{ 'kuiTab-isSelected': mode == 'search' }"
class="kuiTab"
ng-click="mode='search'"
aria-label="List saved searches"
>
Saved Search
</div>
</div>

<div class="list-group-item list-group-item--noBorder" ng-switch-when="visualization">
<saved-object-finder
Expand Down
10 changes: 0 additions & 10 deletions src/ui/public/directives/saved_object_finder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,6 @@ module.directive('savedObjectFinder', function ($location, $injector, kbnUrl, Pr
//key handler for the filter text box
self.filterKeyDown = function ($event) {
switch (keyMap[$event.keyCode]) {
case 'tab':
if (self.hitCount === 0) return;

self.selector.index = 0;
self.selector.enabled = true;

selectTopHit();

$event.preventDefault();
break;
case 'enter':
if (self.hitCount !== 1) return;

Expand Down
14 changes: 13 additions & 1 deletion src/ui/public/styles/dark-theme.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,16 @@
// /src/ui/public/styles/bootstrap/list-group.less
.list-group-item {
background-color: @list-group-bg;
border: 0 none transparent;
border: 1px solid @gray7;
&:nth-child(even) {
background-color: @gray4;
}
}

.list-group-item--noBorder {
border-top: 0;
}

a.list-group-item,
button.list-group-item {
color: @list-group-link-color;
Expand Down Expand Up @@ -224,6 +228,14 @@
}
}

saved-object-finder, paginated-selectable-list {
ul.li-striped {
li {
border: none;
}
}
}

.cell-hover {
background-color: @table-cell-hover-bg;

Expand Down
6 changes: 6 additions & 0 deletions ui_framework/components/tabs/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
$tabBackgroundColor: #FFF;
$tabHoverBackgroundColor: #F2F2F2;

// Dark theme colors
$tabColor--darkTheme: #cecece;
$tabBackgroundColor--darkTheme: #333333;
$tabHoverBackgroundColor--darkTheme: #777777;
$tabBorderColor--darkTheme: #777777;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fantastic! I'd just suggest looking for global color variables (in https://github.com/elastic/kibana/blob/master/ui_framework/components/_variables.scss#L47), to which you can assign these variables. For example:

$tabColor--darkTheme: $globalTextColor--darkTheme;
$tabBackgroundColor--darkTheme: $globalBackgroundColor--darkTheme;
// etc...

This way all of the components will derive the same color palette from a shared source of truth. If there are any colors which don't exist in the _variables.scss file, then feel free to just assign the hex value to a component-specific variable in this file.


@import "tabs";
25 changes: 25 additions & 0 deletions ui_framework/components/tabs/_tabs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
.kuiTabs {
display: flex;
border-bottom: $globalBorderThin;

@include darkTheme {
border-bottom: 1px solid $tabBorderColor--darkTheme;
}
}


Expand All @@ -23,6 +27,11 @@
border-radius: 0; /* 1 */
margin-bottom: -1px; /* 3 */

@include darkTheme {
background-color: $tabBackgroundColor--darkTheme;
border-color: $tabBorderColor--darkTheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll also need to add a color property here to make it match the link's dark theme style (https://github.com/elastic/kibana/blob/master/ui_framework/components/_mixins.scss#L25).

color: $globalLinkColor--darkTheme;

Similar changes will need to be made to the hover and focus states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added colors for the hover states, but the other states I left as is to follow suit with the light theme, which doesn't treat those tabs when they are active as links.

How it looks now:

screen shot 2017-07-31 at 12 23 14 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK cool, I see. I took a look in the browser and it looks like the text color has poor color contrast against the background:

image

I think we should address this color contrast issue. How about using a different variable -- can we add color: $tabColor--darkTheme;? This will get it to look like this:

image

}

& + & {
border-left: none;

Expand All @@ -44,16 +53,32 @@
z-index: 1;
color: $globalLinkColor;
border: 1px solid $globalSelectedBorderColor !important;

@include darkTheme {
color: $tabColor--darkTheme;
background-color: $tabBackgroundColor--darkTheme;
border-color: $tabBorderColor--darkTheme !important;
}
}

&:hover:not(.kuiTab-isSelected) {
color: $globalLinkHoverColor;
background-color: $tabHoverBackgroundColor;

@include darkTheme {
background-color: $tabHoverBackgroundColor--darkTheme;
}
}

&.kuiTab-isSelected {
cursor: default;
color: $globalFontColor;
border-bottom-color: $tabBackgroundColor;

@include darkTheme {
color: $tabColor--darkTheme;
background-color: $tabBackgroundColor--darkTheme;
border-bottom-color: $tabBackgroundColor--darkTheme;
}
}
}
15 changes: 15 additions & 0 deletions ui_framework/dist/ui_framework.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2913,6 +2913,8 @@ main {
display: -ms-flexbox;
display: flex;
border-bottom: 1px solid #D9D9D9; }
.theme-dark .kuiTabs {
border-bottom: 1px solid #777777; }

/**
* 1. Override button styles (some of which are from Bootstrap).
Expand All @@ -2938,6 +2940,9 @@ main {
/* 1 */
margin-bottom: -1px;
/* 3 */ }
.theme-dark .kuiTab {
background-color: #333333;
border-color: #777777; }
.kuiTab + .kuiTab {
border-left: none; }
.kuiTab + .kuiTab:focus:not(.kuiTab-isSelected):not(:active) {
Expand All @@ -2955,13 +2960,23 @@ main {
z-index: 1;
color: #0079a5;
border: 1px solid #0079a5 !important; }
.theme-dark .kuiTab:focus:not(.kuiTab-isSelected):not(:active) {
color: #cecece;
background-color: #333333;
border-color: #777777 !important; }
.kuiTab:hover:not(.kuiTab-isSelected) {
color: #006E8A;
background-color: #F2F2F2; }
.theme-dark .kuiTab:hover:not(.kuiTab-isSelected) {
background-color: #777777; }
.kuiTab.kuiTab-isSelected {
cursor: default;
color: #191E23;
border-bottom-color: #FFF; }
.theme-dark .kuiTab.kuiTab-isSelected {
color: #cecece;
background-color: #333333;
border-bottom-color: #333333; }

/**
* 1. Allow container to deteermine font-size and line-height.
Expand Down