Implement no animation mode by spalger · Pull Request #21629 · elastic/kibana

spalger · 2018-08-03T02:19:15Z

Based on #21747

Introduces a new uiSetting accessibility:disableAnimations, which disabled all non-essential animations in Kibana.

Success Criterion 2.3.3 Animation from Interactions
(Level AAA)

Motion animation triggered by interaction can be disabled, unless the animation is essential to the functionality or the information being conveyed.

To implement this mode the accessibility:disableAnimations advanced config is watched, and when set to true the following styles are injected into the head of the document:

*:not(.essentialAnimation),
*:not(.essentialAnimation):before,
*:not(.essentialAnimation):after {

  /**
   * set the animation/transition duration to 0s so that animation callbacks are
   * still triggered, allowing components that require them to remain functional
   */

  -webkit-animation-duration: 0s !important;
  animation-duration: 0s !important;

  -webkit-transition-duration: 0s !important;
  transition-duration: 0s !important;
}

These styles set the duration of animations/transitions to 0s instead of disabling animations completely with animations: none or something similar to ensure that animation callbacks, required by some components to function, are still called as before.

These styles also allow animations that are essential to the functionality or the information being conveyed to prevent being selected by using the essentialAnimation class on the element that is being styled.

LeeDr · 2018-08-03T13:34:28Z

I'm generally not in favor of tweaking Kibana away from the default settings to run tests. It opens the opportunity for our tests to pass but the un-tweaked behavior to fail for customers.

Animation of EUI components have caused us some pain in automated tests. But I'd prefer we add data-test-subjs to those components that the automation could use to know when the element was ready to use vs changing 30 files to allow disabling them.

Could the EUI components access some global style sheet to enable/disable animations? This seems like the kind of reason we're switching over to using EUI components.

spalger · 2018-08-03T15:18:30Z

Could the EUI components access some global style sheet to enable/disable animations? This seems like the kind of reason we're switching over to using EUI components.

We could, but EUI isn't the only think animating, and the most problematic animation for me are the tooltips in the global nav, which are powered by bootstrap... Turning animations off is globally is so easy I don't feel like it makes sense to limit this to EUI.

LeeDr · 2018-08-05T19:08:58Z

Should the setting be accessibility:enableAnimations that is True by default?

spalger · 2018-08-06T18:28:12Z

It could be, I just prefer false as the default when possible, and I think that disabling animations is more descriptive of the accessibility feature, rather than the ability to turn off the enableAnimations flag...

elasticmachine · 2018-08-06T20:09:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

Tested locally, works great! I had some comments and questions.

cjcenizal · 2018-08-06T20:48:17Z

src/core_plugins/kibana/ui_setting_defaults.js

@gchaps Could you review this please?

I'm thinking that we might not need the description as to why users might want to turn off animations. Can we simplify to:

Turn off all unnecessary animations in the Kibana UI. Refresh the page to apply the changes.

cjcenizal · 2018-08-06T20:58:49Z

test/common/services/es_archiver.js

More of an observation than a comment... this code really requires an understanding of what stats is, what its created property represents, what kibanaServer is, what its uiSettings property is, and what calling update with an empty object will do wrt overrides and the Kibana index. I don't think any of this would be apparent without this comment, so the comment is great but ideally the code would be a little more transparent, too.

cjcenizal · 2018-08-06T21:11:26Z

test/common/services/es_archiver.js

Why do we need to do this? Is it not enough to set it in ui_settings.js?

Well, the esArchiver wipes out the .kibana index on a regular basis, which is why I added the ability for test configs that use the esArchiver to automatically update the uiSettings without needing to call uiSettings.update() after each esArchiver.load(). I'm going to fix the config name though to not explicitly mention disabling animations, as what it's really doing is enforcing the overrides in the kibanaServer.uiSettings service.

cjcenizal · 2018-08-06T21:12:16Z

src/ui/public/style_compile/style_compile.css.tmpl

Since we're not injecting any JS variables here, would it make more sense to use a system similar to how we apply EUI themes?

Here's a reference: https://github.com/elastic/kibana/pull/15657/files#diff-8483334633f95ed911cae02b98f54f02R15

Yeah, I considered something like that, but none of the existing style-inclusion features really seemed to fit in my eyes, but this one felt the most similar because it's dynamically defining the css in a style block based on config.

Maybe I could add a theme.extendTheme(css) method that will include additional css in the theme style element every time, regardless what the theme is, or maybe I could use the the "usable" feature of the webpack style-loader to dynamically add and remove just a single style sheet, or maybe we just want a module in ui/styles somewhere that watches the config and adds/removes an element from the <head> based on the config... Which would you choose?

a module in ui/styles somewhere that watches the config and adds/removes an element from the based on the config

This sounds the simplest to me, though I think adding a specific node to ui_render/views/chrome.pug for mounting the styles would be a good idea, to make the CSS load-order transparent.

elasticmachine · 2018-08-06T22:29:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-08-06T22:30:43Z

Weird selenium failure:

21:17:28 Running script [build] in [x-pack]:
21:17:28 
21:17:28 yarn run v1.6.0
21:17:28 $ gulp build
21:17:35 [21:17:35] Using gulpfile ~/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/x-pack/gulpfile.js
21:17:35 [21:17:35] Starting 'clean-test'...
21:17:35 [21:17:35] Starting 'report'...
21:17:35 [21:17:35] Starting 'prepare'...
21:17:35 [21:17:35] Finished 'clean-test' after 31 ms
21:17:35 [21:17:35] Starting 'clean'...
21:17:35 [21:17:35] Finished 'clean' after 23 ms
21:17:35 [21:17:35] Package Name x-pack
21:17:35 [21:17:35] Version 7.0.0-alpha1-SNAPSHOT
21:17:35 [21:17:35] Build Number 18044
21:17:35 [21:17:35] Build SHA 9a26908b9e0e0299040d64026fa34bde74722be3
21:17:35 [21:17:35] Finished 'report' after 494 ms
21:17:39 [21:17:39] 'prepare' errored after 4 s
21:17:39 [21:17:39] Error: Error downloading browsers, checksums incorrect for:
21:17:39     - https://github.com/Medium/phantomjs/releases/download/v2.1.1/phantomjs-2.1.1-windows.zip => /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/x-pack/plugins/reporting/.phantom/phantomjs-2.1.1-windows.zip
21:17:39     - https://github.com/Medium/phantomjs/releases/download/v2.1.1/phantomjs-2.1.1-linux-x86_64.tar.bz2 => /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/x-pack/plugins/reporting/.phantom/phantomjs-2.1.1-linux-x86_64.tar.bz2
21:17:39     at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.js:68:13
21:17:39     at <anonymous>
21:17:39     at process._tickCallback (internal/process/next_tick.js:188:7)
21:17:46 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
21:17:46 error Command failed with exit code 1.
21:17:46    │ERROR  failure
21:17:46    │ERROR  Error: Command failed: yarn run build
21:17:46    │           at makeError (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/packages/kbn-pm/dist/index.js:35523:9)
21:17:46    │           at Promise.all.then.arr (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/kibana/packages/kbn-pm/dist/index.js:35628:16)
21:17:46    │           at <anonymous>
21:17:46    │           at process._tickCallback (internal/process/next_tick.js:188:7)
21:17:46

cjcenizal · 2018-08-06T22:33:16Z

Retest

cjcenizal

Thanks for making that change to esArchiver! It's much easier for me to follow the code now.

cjcenizal · 2018-08-06T23:15:08Z

test/common/services/kibana_server/extend_es_archiver.js

Would it make sense to just pass defaults and uiSettings as arguments into extendEsArchiver? Seems like a bit of a code smell to pass someone a tool-fetching assistant when you could just hand them the tool they want.

Yeah, that was my original implementation, but the kibanaServer service might not be available, so I'm relying on the fact that the uiSettings.defaults setting is used to determine that I should load the kibanaServer service. This meant that I had to encode into the esArchiver when the kibanaServer would be interested in extending it. Rather than basing it on the existence of the uiSettings.defaults config I had added a hasService() method to the service loader that esArchiver was using to check for the kibanaServer service before calling it's extendEsArchiver() function... But that felt like a lot of changes just to avoid passing the getService() function to extendEsArchiver(). Happy to go back to it if you think that's a better approach.

Welp, thought about it again and it really wasn't that much trouble, implemented in c149e11

Oh I see. I think it's OK as-is. Could you leave a comment on line 31 to explain this relationship?

// The kibanaServer service won't be available unless uiSettings.defaults has been set. const { uiSettings } = getService('kibanaServer');

elasticmachine · 2018-08-07T02:06:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-07T18:57:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-07T19:13:16Z

retest

elasticmachine · 2018-08-07T20:42:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-07T22:53:26Z

retest

elasticmachine · 2018-08-08T00:16:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-08T01:47:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-08T02:08:21Z

retest

elasticmachine · 2018-08-08T03:38:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-20T22:33:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…al tests

elasticmachine · 2018-08-21T02:18:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…al tests (elastic#21629)

…al tests (#21629) (#22203)

spalger · 2018-08-22T21:56:16Z

6.x/6.5: 138f456

spalger added the WIP Work in progress label Aug 3, 2018

spalger mentioned this pull request Aug 3, 2018

[uiSettings] support overriding uiSettings from the config file #21628

Merged

This comment has been minimized.

Sign in to view

spalger force-pushed the implement/no-animation-mode branch from 003c1c4 to 7f12510 Compare August 3, 2018 14:38

spalger force-pushed the implement/no-animation-mode branch 2 times, most recently from bac1c66 to aef0b6a Compare August 3, 2018 19:59

elastic deleted a comment from elasticmachine Aug 3, 2018

This comment has been minimized.

Sign in to view

cjcenizal mentioned this pull request Aug 6, 2018

Fix flaky test by verifying that a search has been saved by checking for a success toast #21302

Closed

cjcenizal mentioned this pull request Aug 6, 2018

Support no-animation mode for accessibility elastic/eui#1089

Closed

spalger mentioned this pull request Aug 6, 2018

[dashboard/fullScreen] override z-index override #21704

Merged

spalger force-pushed the implement/no-animation-mode branch from 5f59a47 to 9a26908 Compare August 6, 2018 21:11

cjcenizal reviewed Aug 6, 2018

View reviewed changes

cjcenizal approved these changes Aug 6, 2018

View reviewed changes

cjcenizal reviewed Aug 6, 2018

View reviewed changes

spalger force-pushed the implement/no-animation-mode branch from f800f35 to b961991 Compare August 6, 2018 23:46

This comment has been minimized.

Sign in to view

spalger force-pushed the implement/no-animation-mode branch from a4cd844 to f3abe1d Compare August 7, 2018 00:07

spalger added the review label Aug 7, 2018

This comment has been minimized.

Sign in to view

spalger force-pushed the implement/no-animation-mode branch from dc1fc1c to e06fdb1 Compare August 7, 2018 17:26

spalger added WIP Work in progress and removed review labels Aug 7, 2018

spalger mentioned this pull request Aug 8, 2018

[retry] implement waitFor method #21747

Merged

spalger removed the v6.6.0 label Aug 14, 2018

spalger force-pushed the implement/no-animation-mode branch from 7cb23e9 to 4fdbadc Compare August 20, 2018 21:21

[accessibility] implement no-animation mode, auto enable for function…

eedef9e

…al tests

spalger force-pushed the implement/no-animation-mode branch from 4fdbadc to eedef9e Compare August 21, 2018 00:34

spalger added review and removed WIP Work in progress labels Aug 21, 2018

spalger requested review from azasypkin and removed request for azasypkin August 21, 2018 02:29

spalger merged commit 9209987 into elastic:master Aug 21, 2018

spalger pushed a commit to spalger/kibana that referenced this pull request Aug 21, 2018

[accessibility] implement no-animation mode, auto enable for function…

50c29a7

…al tests (elastic#21629)

spalger mentioned this pull request Aug 21, 2018

[6.x] [accessibility] implement no-animation mode, auto enable for functional tests (#21629) #22203

Merged

spalger pushed a commit that referenced this pull request Aug 22, 2018

[accessibility] implement no-animation mode, auto enable for function…

138f456

…al tests (#21629) (#22203)

spalger deleted the implement/no-animation-mode branch August 22, 2018 21:56

LeeDr mentioned this pull request Aug 22, 2018

Speed up dashboard add panel #22278

Merged

Conversation

spalger commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

LeeDr commented Aug 3, 2018

Uh oh!

spalger commented Aug 3, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

LeeDr commented Aug 5, 2018

Uh oh!

spalger commented Aug 6, 2018

Uh oh!

elasticmachine commented Aug 6, 2018

💔 Build Failed

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 6, 2018

💔 Build Failed

Uh oh!

cjcenizal commented Aug 6, 2018

Uh oh!

cjcenizal commented Aug 6, 2018

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

Uh oh!

This comment has been minimized.

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 7, 2018

Uh oh!

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 7, 2018

Uh oh!

elasticmachine commented Aug 8, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Aug 8, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 8, 2018

Uh oh!

spalger commented Aug 3, 2018 •

edited

Loading