Palette swap for accessibility / kibana 6 #12085

snide · 2017-05-30T22:13:40Z

Replaces #11610

Description

This PR reskins a decent portion of Kibana to be mostly AA accessibility compatible in regards to coloring of major navigation elements.

A note on our less / sass and variables

Kibana has a pretty messy css compile path with both less and sass in play. Right now we have a couple parallel build processes that don't talk to each other all that well. The best way I could think to uniformly style them all was to add some global variables to the less and sass files and then carry that through the existing structure. In general I think this method is a band-aid to get the job done. I think a decent portion of the core css systems need to be rebuilt, and at that time, a proper variable / theming structure should be put in place.

A note to reviewers

I tried to avoid touching the template layer when possible, and tried to keep my changes to the css layer only. There are a few cases where I had to touch templates though, and since I'm not as familiar with the areas I'm touching please take a look at those first.

I'd also mention that these color changes may look more minimal than they actually are. Again, I don't know every corner of Kibana, so if you notice a color problem, please take a screen shot and give me a short note on how to recreate it.

Addresses #11521

Release Note: We re-skinned a decent portion of Kibana to be mostly AA accessibility compatible in regards to coloring of major navigation elements.

cjcenizal · 2017-05-30T22:43:23Z

Weird functional test failure:

10:34:18.011  warn  Failure loading service "remote"
10:34:18.014 ERROR  Error: remote failed to start within 2 minutes
10:34:18.014            at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:13:13
10:34:18.014            at undefined.next (native)
10:34:18.015            at step (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1)
10:34:18.015            at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1
Warning: Error: Failure initializing 1 service(s)
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/src/functional_test_runner/lib/providers/provider_collection.js:48:13
    at undefined.next (native)
    at step (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/src/functional_test_runner/lib/providers/provider_collection.js:2:1)
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/src/functional_test_runner/lib/providers/provider_collection.js:2:1� Use --force to continue.

jenkins, test this

snide · 2017-05-31T18:03:43Z

@cjcenizal Still working through dark mode and some small stuff around the panels within the tables, but feel free to start poking at this.

cjcenizal · 2017-05-31T20:57:16Z

You should be able to rebase master onto this branch to get the tests passing. Shouldn't be any conflicts this time! But if there are... you know what to do. 💥 🔫

cjcenizal · 2017-06-01T19:52:24Z

Link contrast

When someone is fully color-blind, it's pretty tough to differentiate links from regular text. The obvious solution is to add an underline, but if we can't do that maybe we can change the lightness.

Dark theme

The query bar doesn't contrast very well against the background. Maybe we should use an outline treatment, similar to the light theme?

The text and other inputs also looks a little low-contrast. Maybe we bump up the contrast on the text and add outlines around the inputs.

The "Set to now" buttons look a little weird, and maybe we can create a dark theme version of checkboxes to use in the Datepicker?

Looks like the focus state of the Datepicker button is also incorrect:

Spy

Looks like the spy puts gray text on gray background in a couple places:

Discover

The color highlighting of the JSON view looks a little desaturated.

Maybe we can try matching the colors in Console?

The existing treatment of the field names in the table applies a semi-transparent background colors so it blends with the blue highlight of the row in the Context Log. We should probably preserve something like that to avoid this kind of vibrating contrast (btw the +/- button backgrounds are being fixed by @weltenwort in #11819):

Visualize

The index pattern bar really stands out! Can we make this a little more subdued? I'm not sure why it needs to be so high in the visual hierarchy.

There's quite a bit of vertical white-space between some sections. Maybe it can be reduced?

Does the vertical line denoting sections need to be blue? It seems like this is our attention-getting color (sidebar, primary buttons). Might be better to make this gray instead?

TSVB

The axis labels and legend labes in TSVB are really light and hard to read.

Dashboards

You mentioned this already, that there's some weirdness with the way our tables appear when they have a feedback message.

On a semi-related note, I love how you treated the "dashboard is empty" message. How would you feel about turning this into a component in the UI Framework, and reusing it in tables?

Dev Tools

The light blue (#40b0d3) on gray (#dbdcdd) fails the WebAIM Contrast Checker:

Looks like this color scheme is reused in the history:

epixa · 2017-06-01T20:12:43Z

One thing I'd like to urge here is separating feedback into "must have to make this as good if not better than what we have now" and "must be done for compliance reasons". We can continue to iterate on the latter after this PR gets merged, whereas we don't want things to get worse when we merge this PR. Progress over perfection, etc. etc.

cjcenizal · 2017-06-01T21:43:31Z

jenkins, test this

snide · 2017-06-05T19:52:36Z

@cjcenizal Think I got most of these. I'd like to avoid styling our syntax highlighters just yet. We use quite a few different ones and it would probably take me a bit to get that stuff where I'm happy with it.

TSVB uses canvas and while I'd love to dig deep into @simianhacker's codebase it would likely similarly take me awhile to get those visuals consistent with everything else. I'll schedule out a review with him later and I can tackle that in a separate PR. Think since we label that one with "experimental" right now anyway it's OK to wait on. I wouldn't mind spending time working on that UI as a full PR of work. As it is now I've left it mostly unchanged.

cjcenizal · 2017-06-06T05:21:07Z

Looks like the Visualize functional tests are failing because it can't find some elements. My guess is that some of the selectors in the page object use CSS class names to find elements, but those class names changed? I can take a closer look tomorrow.

cjcenizal · 2017-06-08T04:59:04Z

Discover

LocalNav

When the query bar has focus state, the "Uses lucene query syntax" link is invisible.

The "Add filter" link doesn't have a very clear focus state.

Table

The tooltip in the chart has labels that are pretty hard for me to read. This is backed up by the WebAIM contrast checker, which fails AAA for #3394b7 on #00090C. Why use blue in this situation? Can we use white with a bold treatment, like the way the rest of our labels are treated?

Context Log

The backgrounds of the +/- icons are white. I think this is fixed by #11819, so we're semi-blocked by that PR.

snide · 2017-06-08T14:58:38Z

The all white coloring isn't enough definition between the elements. It passes AA, which is what is our goal was. Gonna leave that one as is, or possibly lighten the blue.

I'll fix the focus on the filter. Looks like someone added an overflow on the surrounding block (which hides the shadow). Can't see any other reason other than they wanted to avoid dealing with a vertical height issue.

Will also fix the query syntax focus. Good catch.

snide · 2017-06-08T17:59:11Z

@cjcenizal Issues are fixed outside of that one outside PR.

snide · 2017-06-08T18:35:32Z

src/ui/public/visualize/visualize.less

  display: flex;
  flex-direction: column;

-  height: 482px;


@cjcenizal @thomasneirynck

I couldn't figure out why this rule existed and where such a specific number came from. I made some changes to the overflow / height states of the spy to make it actually usable within discovery. I checked that this doesn't effect the spy within dashboard and the visualize creation itself, but wanted to pass it by you just in case there's some part of Kibana I missed that actually requires this to exist.

Looks like it was just old code.

cjcenizal · 2017-06-08T23:21:30Z

This is looking fantastic! Thanks for improving the legibility of the tooltip labels. As a user, I don't have such a hard time reading them now.

I've organized the things I've found into "Blockers", "Nice-to-have", and "Do later" buckets, based on @epixa's comment above.

Blockers

Tabs change in height when focused.

This is caused by padding-bottom: 11px; /* 4 */ on line 46 of _tabs.scss in the UI Framework. If we remove that line and the corresponding comment we should be fine. Marked a blocker because this doesn't happen in master.

Index pattern focus state isn't visible in "Index Patterns" side bar.

Referring to the "logstash-*" button below the "Create index pattern" button. This focus state is easy to see in master.

Timepicker checkbox focus states aren't visible.

Referring to the "round to the minute/second" checkbox. I think this a blocker because they're visible in master.

kuiLocalDropdownInput elements don't have visible focus states.

This is a global thing so we should probably address this in the UI Framework. I think it's a blocker because the focus states are visible in master.

Discover sidebar checkboxes don't have visible focus states.

The states are visible on master so it's a blocker.

Nice-to-have

Timepicker day focus state is hard to see in dark theme.

As a user, it's easier for me to see the lighter blue in master. Didn't look up the math.

Timelion text area and buttons have double-outlines when focused.

It seems like their classes have focus states, but they're also inheriting a global *:focus selector. Marked as nice-to-have because it looks a little weird, but it's still functional... a little too functional! 😄

Do later

Table and ToolBar use slightly different gray colors for their borders.

They're just barely different so I think we can just do this later.

Bootstrap tabs have weird focus states.

They already have this weird focus state so I think we can address it later.

Visualization panel "full-screen" button is hard to see in dark theme.

Blue on dark gray is difficult to read. We probably just need better dark theme styles for the hollow button.

Spy panel button isn't visible in dark theme.

This exists in master already.

Visualize "Play" and "Reset" button focus states aren't visible.

This behavior exists in master already and also be addressed by #11094.

snide · 2017-06-09T04:17:52Z

@cjcenizal All blockers are addressed (and a couple non-blockers as well). I couldn't replicate the toaster one. You'll need to provide the pattern to get to it.

snide · 2017-06-12T17:19:56Z

@epixa Rebased. Unfortunately this branch touches so much it slips into conflict pretty regularly anytime css is changed.

Side note, would love to not commit compiled CSS into master for this reason. Be nice if that was just a part of our build process.

epixa · 2017-06-12T17:24:06Z

@snide Understood. All the more reason to get this thing in asap. Keeping it open is a drain.

I completely agree about not checking in the css. The current build system muddies the water between what is a behavior in release builds and what is a behavior for development, so the only way to not commit the css would be to ship sass with releases, but that's not doable since sass is a native module and thus needs to be compiled on the OS that it is intended to run on. The new build system will address this.

epixa · 2017-06-12T17:36:14Z

Looks like there was a network error in CI when trying to get the latest node builds.

jenkins, test this

epixa

There are minor style details here and there that we should improve upon, but I didn't see anything that warrants blocking this PR in particular. With a PR like this, far more issues will surface when people start using it than they will in a single PR review.

LGTM

cjcenizal

Beautiful work @snide. I know you think the CSS actually got worse, but I think the addition of new variables and the new rules which tie certain parts of the UI is a step in the right direction.

I had a few very minor comments and then I think we're good to go. I'll start moving the unaddressed visual items into their own issues.

cjcenizal · 2017-06-12T18:50:37Z

src/core_plugins/kibana/public/visualize/editor/sidebar.html

-            <a class="vis-editor-subnav-link vis-editor-subnav-link--danger">
-              <span class="kuiIcon fa-warning"></span>
+            <a class="kuiButton kuiButton--danger navbar-btn-link">
+              <i class="fa fa-warning"></i>


This should be <span> since <i> is non-semantic.

cjcenizal · 2017-06-12T18:55:34Z

src/core_plugins/kibana/public/visualize/editor/styles/_editor.less

-        margin-left: -@vis-editor-agg-editor-spacing;
+        // margin-bottom: -@vis-editor-agg-editor-spacing - 1; // extra one pixel covers the aggs bottom border
+        // margin-right: -@vis-editor-agg-editor-spacing;
+        // margin-left: -@vis-editor-agg-editor-spacing;


I think we can delete this entire class.

We can also remove it from the markup on line 93 of agg.html.

cjcenizal · 2017-06-12T18:58:14Z

src/ui/public/doc_table/components/table_row/details.html

      data-test-subj="docTableRowAction"
      ng-href="{{ getContextAppHref() }}"
      ng-if="indexPattern.isTimeBased()"
    >
      View surrounding documents
    </a>
    <a
-      class="kuiLink documentTableRow__action"


Can we delete the .documentTableRow__action class on line 15 of table_row.less?

cjcenizal · 2017-06-12T19:13:10Z

src/ui/public/styles/base.less

+  border-radius: 4px;
+}
+
+.kuiSideBarSection .kuiSideBarSection .kuiSideBarSection{


Can we either create a class for this and update the markup, or create a brief ticket so we don't forget? I think this one stands out because the solution seems pretty trivial, and the existing selector is potentially pretty confusing for people.

I'd rather do this in a separate refactor. Currently those three tabs are all using different markup and I'm faking them all to look the same with separate css. This whole bit needs a lot of work and because of the way these templates sub nested each other I didn't have enough angular to better specify the classes (if I recall correctly).

cjcenizal · 2017-06-12T19:15:01Z

src/ui/public/vis/components/color/seed_colors.js

- * Using a random color generator presented awful colors and unpredictable color schemes.
- * So we needed to come up with a color scheme of our own that creates consistent, pleasing color patterns.
- * The order allows us to guarantee that 1st, 2nd, 3rd, etc values always get the same color.
- */


Why remove this comment?

cjcenizal · 2017-06-12T19:25:01Z

ui_framework/components/panel/_panel.scss

 }

 .kuiPanel--withHeader {
+  // border-top: none;


I think we can delete this class.

Also, we need to replace kuiPanel--withHeader with kuiPanel--withToolbar in two places:

Line 15 of src/core_plugins/kibana/public/management/landing.html

Line 1 of src/ui/public/table_info/table_info.html

Also, the border for the Panel is #E4E4E4 but the border for the ToolBar is #D9D9D9. We should probably use the same value for both.

cjcenizal · 2017-06-12T19:25:31Z

ui_framework/components/panel/_panel.scss

+  // border-top: none;
+}
+
+.kuiPanel--withToolbar {


Can we rename this kuiPanel--withToolBar (capital B) to be consistent with the casing of the ToolBar component?

cjcenizal · 2017-06-12T19:29:30Z

ui_framework/components/button/button.js

@@ -26,6 +26,7 @@ const buttonTypeToClassNameMap = {
  danger: 'kuiButton--danger',
  warning: 'kuiButton--warning',
  primary: 'kuiButton--primary',
+  secondary: 'kuiButton--secondary',


I love that you're updating the React components too! On line 8 we need to make a change to match the change you've made here. Line 8 needs to be updated to be:

const BUTTON_TYPES = [ 'basic', 'hollow', 'danger', 'warning', 'primary', 'secondary', ];

This defines the allowed values, so now "secondary" will be accepted as a valid type.

cjcenizal

This looks so frickin' good dude!

Just a couple last things and then let's merge this sucker:

We can delete the .kuiPanel--withHeader class from the UI Framework.
We need to update components/empty_table_prompt/empty_table_prompt_panel.js to use the kuiPanel--withToolBar class instead of the old withHeader class.

cjcenizal · 2017-06-13T00:02:35Z

Also you need to run node scripts/jest --coverage and commit the results, to update the tests for the React Button component you changed. This will get the tests passing.

snide · 2017-06-13T02:51:28Z

@cjcenizal Done. We should find one more commit for good luck.

cjcenizal · 2017-06-13T05:20:08Z

jenkins, test this

epixa · 2017-06-13T18:48:33Z

The top border on management sections is missing:

The tabs on the saved objects list are separated from the content, which looks strange:

I'm fine with not blocking this PR on either of these if we follow up with fixes after merging.

LGTM

snide · 2017-06-13T19:11:56Z

@epixa Fixed the management panels. The tabbing stuff is more of design problem that exists in master as well. Nothing really changed there (agree it's really awkward though).

snide changed the title ~~11521/colors again~~ [WIP] Palette swap for accessibility / kibana 6 May 30, 2017

snide mentioned this pull request May 30, 2017

WIP Accessibility palette swap #11610

Closed

snide added Project:Accessibility Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.0.0-alpha2 v6.0.0 and removed v6.0.0-alpha2 labels May 30, 2017

snide requested a review from cjcenizal May 31, 2017 18:02

snide requested a review from epixa May 31, 2017 18:03

snide force-pushed the 11521/colors_again branch from cf36ca6 to da42250 Compare June 1, 2017 16:40

snide changed the title ~~[WIP] Palette swap for accessibility / kibana 6~~ Palette swap for accessibility / kibana 6 Jun 1, 2017

snide force-pushed the 11521/colors_again branch from 776e86d to 49a193c Compare June 2, 2017 07:39

snide mentioned this pull request Jun 2, 2017

Improve the default colors of the charts #12024

Closed

snide force-pushed the 11521/colors_again branch from eae0931 to 9297f76 Compare June 5, 2017 16:00

snide force-pushed the 11521/colors_again branch from c4723a6 to a783b83 Compare June 6, 2017 16:03

snide commented Jun 8, 2017

View reviewed changes

cjcenizal added the review label Jun 8, 2017

snide added 5 commits June 12, 2017 10:13

fix line height on vis tooltip

509973a

fix index focus, tab focus

86e661e

fix bootstraps focus states for checkboxs and radios

a0f71ec

bootstrap focus, local nav input focus coloring

f8bd667

focus borders on kui selects and textareas

495af49

snide force-pushed the 11521/colors_again branch from 4b57f8c to 495af49 Compare June 12, 2017 17:17

epixa approved these changes Jun 12, 2017

View reviewed changes

cjcenizal suggested changes Jun 12, 2017

View reviewed changes

snide added 2 commits June 12, 2017 13:47

address color feedback, mostly around naming and kui cleanup

d2894fa

fix focus state with links in notification messages

cb7ef5d

cjcenizal approved these changes Jun 13, 2017

View reviewed changes

panel withHeader removed completely, tests updated

e8fdb0f

fix management panel bordering

25d784a

snide merged commit dedef14 into elastic:master Jun 13, 2017

snide deleted the 11521/colors_again branch June 13, 2017 21:06

cjcenizal mentioned this pull request Jun 21, 2017

Accessibility: Color contrast #11521

Closed

cjcenizal added the release_note:enhancement label Jul 10, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

aphelionz mentioned this pull request May 7, 2018

Accessibility: Kibana 6.3 Meta Issue #18866

Closed

48 tasks

wylieconlon mentioned this pull request Mar 31, 2020

Visualizations should default to EUI palette instead of 7-color custom palette #62085

Closed

Palette swap for accessibility / kibana 6 #12085

Palette swap for accessibility / kibana 6 #12085

Conversation

snide commented May 30, 2017 • edited by jimgoodwin Loading

Description

A note on our less / sass and variables

A note to reviewers

cjcenizal commented May 30, 2017

snide commented May 31, 2017

cjcenizal commented May 31, 2017

cjcenizal commented Jun 1, 2017

Link contrast

Dark theme

Spy

Discover

Visualize

TSVB

Dashboards

Dev Tools

epixa commented Jun 1, 2017

cjcenizal commented Jun 1, 2017

snide commented Jun 5, 2017 • edited Loading

cjcenizal commented Jun 6, 2017

cjcenizal commented Jun 8, 2017 • edited Loading

Discover

LocalNav

Table

Context Log

snide commented Jun 8, 2017

snide commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jun 8, 2017 • edited Loading

Blockers

Nice-to-have

Do later

snide commented Jun 9, 2017

snide commented Jun 12, 2017

epixa commented Jun 12, 2017

epixa commented Jun 12, 2017

epixa left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal commented Jun 13, 2017 • edited Loading

snide commented Jun 13, 2017

cjcenizal commented Jun 13, 2017

epixa commented Jun 13, 2017

snide commented Jun 13, 2017

snide commented May 30, 2017 •

edited by jimgoodwin

Loading

snide commented Jun 5, 2017 •

edited

Loading

cjcenizal commented Jun 8, 2017 •

edited

Loading

cjcenizal commented Jun 8, 2017 •

edited

Loading

cjcenizal Jun 12, 2017 •

edited

Loading

cjcenizal commented Jun 13, 2017 •

edited

Loading