Reduce animation delays

I'd generalize this to saying "fix all of Quick Edit's animations" — we already had an issue for this: #2080205: [META] Fix/finalize CSS transitions or get rid of them, with #2080201: Padding/unpadding of textual fields is no longer animated, feels jerky as a child issue.

I'm open to your suggestions; I'm willing to show/explain you where/how to change all of Quick Edit's transitions/animations and help you get this done.

Totally agree with all of this.

Additionally there is a "jump" When the toolbar moves to a new field because it re-adjusts it's position after the padding has been added. Simple solution would be add the padding first then move the toolbar down.

Adding the padding is enough visual affordance that you have re-focused to that field.

I'm also ok with calculating the width first and displaying the toolbar at the correct width so we don't see it "growing". That is unnecessary and distracting to users.

I'll try to help out on this issue. @Wim Leers will you show/explain where/how to disable the transitions/animations? I've disabled all the rules in the Animations section in quickedit.theme.css, switched to the Stark theme, unaggregated css/js and did cache-rebuild, but the animations remain unaffected.

I agree the animations will become tedious and should be removed or accelerated greatly.

Thanks

Should probably toggle animations based on jQuery.fx.off === true if possible. See #2312805: Tour does not support turning off animation.

Linking to a related issue on the style of the bar: [#2315915}

Another +1 for simpler animation here. My observations:

Opening

Both the scaling to the right and the small move upwards seem unnecessary indeed.

Moving between fields

- Overall feel could be snappier indeed, a bit more aggressive ease & ease out.
- Contrary to this, it might be good to wait a bit longer before actually starting the animation to another editable item, because what I see is that the toolbar wants to reset, move back to the title at the top.
- Actually, why does it do this at all (move back to the title)?

Attaching a short video to illustrate those last two points: if I move a bit slowly between author name and date you see the toolbar move back to the title for the time when the mouse is not over an editable part (this is with latest Firefox on OS X).

Hi @Bojhan,

The toolbar animations for the Quickedit module resides in the quickedit.theme.css file (core/modules/quickedit/css/quickedit.theme.css). Checkout line 101 for the toolbar container - where the transition is defined. Currently this is set for 1s duration. You're certainly right about tightening this up, setting this property to even half makes sense.

Slightly related why do we only define the -webkit- prefix? Firefox and Opera also provide browser compatibility transition prefixes.

Thanks, @swim!

@Bojhan, does that give you enough info to roll a patch?

Something to have a go at during devdays maybe? :)

Working on this with @shaal at #FLDC17

@shaal and I reduced the animation to 0.2s. Note that we also changed the width of the quickedit toolbar when the admin toolbar is open in vertical mode. This is because the admin toolbar would push over the quickedit toolbar, making the user unable to X out of it at small viewport widths. We also removed unnecessary JavaScript and moved the logic to CSS. The JS was calculating the width of the window, as opposed to the dimensions of the field (as indicated by the comments).

Working on this with @mherchel #FLDC17

Woah, #18 introduces big changes. That will definitely need manual testing :)

Thanks for taking this on!

A minor issue I noticed was that the overlay was not being repositioned when the orientation of the toolbar was changed.

The overlay should now automatically adjust whenever the orientation is changed. Additionally, 0.2 felt very fast, moving to 0.3 seconds keeps the massive speedup, but makes a bit smoother.

@wim-leers: Thanks! What else needs to happen to move this on. The only changes from the user's point of view should be a faster animation speed. The removal of the JS is code that should be in CSS anyway, since it's not dynamic.

@drpal: Good catch, but that issue isn't introduced by this patch - it's present in the current 8.4.x. Attaching a gif after a git pull (and without the patch)

Should we open another issue?

@mherchel

Sure, I can open another issue and add the patch. #2856364: Reposition Quickedit overlay on toolbar orientation change

@mherchel Animation delays is looks good now ;) Thanks for working out on this.

One thing i have noticed that when we were on description field and if we move pointer directly to top field, over to the quick edit, it is not moving to its default position.
Please look into screencast for better understanding.

@Manjit.Singh that's a good point, but that behavior also happens in the latest Drupal 8.4.x HEAD and isn't introduced in this patch. I threw up a quick screencast on non-patched drupal 8.4.x at https://youtu.be/fA2Yxqj1hP0 to show the same behavior of Drupal 8.4.x HEAD

In my opinion, I think the current behavior is correct (because the intent of the user may be to interact with the toolbar). But, feel free to create additional issue for more discussion. Thanks for the review though :)

@Manjit.Singh Looks like this issue is present prior to this patch. Would you mind opening another issue so we can discuss further? Thanks.

Yep. It's a hover/event firing on the correct element issue. However I do think it's the correct behavior.

#21 still includes the changes now moved to #2856364: Reposition Quickedit overlay on toolbar orientation change. Will need a reroll for that.

What else needs to happen to move this on. The only changes from the user's point of view should be a faster animation speed. The removal of the JS is code that should be in CSS anyway, since it's not dynamic.

Well, jesse.beach originally wrote the code that you now deleted. She added it at #1678002-134: Edit should provide a usable entity-level toolbar for saving fields. (Yes, that required me to manually open a bunch of those patch files to find the first one that contained it.) Sadly, no extra background information or rationale there.

+++ b/core/modules/quickedit/css/quickedit.theme.css
@@ -102,8 +102,14 @@
+.toolbar-vertical.toolbar-tray-open .quickedit-toolbar-container {
+  width: calc(90% - 240px);
 }
 .quickedit-toolbar-container > .quickedit-toolbar-content {

This should have some documentation. You wrote Note that we also changed the width of the quickedit toolbar when the admin toolbar is open in vertical mode. This is because the admin toolbar would push over the quickedit toolbar, making the user unable to X out of it at small viewport widths — can you summarize that so that future maintainers won't be cursing that this is incomprehensible? :)

We also removed unnecessary JavaScript and moved the logic to CSS. The JS was calculating the width of the window, as opposed to the dimensions of the field (as indicated by the comments).

Great catch! The comment indeed didn't match the code. I'm no CSS expert, so please bear with me… but as far as I can tell, the CSS and JS are not actually equivalent? How is the CSS still ensuring that on clients with narrow viewports, the min and max width are adjusted appropriately?

I've rerolled this to not include the changes from #2856364: Reposition Quickedit overlay on toolbar orientation change, this does however still include the increase on the animation length to 0.3s.

Thanks @drpal! You beat me to it :)

@wim-leers: here's the explanation of the removal of the JS and addition of the CSS:

.css({
  'max-width': (document.documentElement.clientWidth < 450) ? document.documentElement.clientWidth : 450,

This block of JS sets the the toolbar max-width to 100% if the window width is less than 450px, and sets it to to 450px otherwise.

The same thing can be accomplished by setting max-width to 450px in CSS, since the toolbar will never approach that when the window width is less than that.

// Set a minimum width of 240px for the entity toolbar, or the width
// of the client if it is less than 240px, so that the toolbar
// never folds up into a squashed and jumbled mess.
'min-width': (document.documentElement.clientWidth < 240) ? document.documentElement.clientWidth : 240,

This block of JS sets the toolbar's min-width to either the window width, or 240px.

The same thing can be accomplished by setting the min-width to 240px in CSS, since it should never go below that in any circumstance.

'width': '100%'

This just sets the toolbar width to be 100%, which tells the toolbar to fill up all of the space up until either 1) max-width is reached, or 2) 100% of the containing element is reached (which will be the same as the window width).

With all of that in mind, you can replicate the functionality in standard CSS using

min-width: 240px;
max-width: 450px;
width: 100%;

After doing that, we noticed an issue where the toolbar could be pushed off the screen when the toolbar was in vertical mode and was a small viewport. We used the CSS calc() function to accommodate the toolbar (which is a static width).

.toolbar-vertical.toolbar-tray-open .quickedit-toolbar-container {
  width: calc(90% - 240px);

Let me know if there are any follow up questions, or if something isn't quite clear.

Thanks for #29!

Can we move as much of #29 as possible into comments for the CSS? So that the next person doesn't have to do git blame, find this issue, find #29, and then go "Ahaaaaaaa"? :)

After that, I'm happy to RTBC!

Attached. Thanks Wim!

Very sorry… but I think we're still losing documentation here. I'm afraid I have to insist on this, otherwise we make future maintenance harder, not easier

1. +++ b/core/modules/quickedit/css/quickedit.theme.css
   @@ -102,8 +102,20 @@
   +  /* Ensure toolbar stays within logical width. */
   +  min-width: 240px;
   +  max-width: 450px;
   +  width: 100%;
   

   We don't have docs for this 240px and 450 px stuff.

2. +++ b/core/modules/quickedit/js/views/EntityToolbarView.js
   @@ -323,16 +323,6 @@
   -            // Set a minimum width of 240px for the entity toolbar, or the width
   -            // of the client if it is less than 240px, so that the toolbar
   -            // never folds up into a squashed and jumbled mess.
   

   But we used to ahve that here.

   Let's move this comment from the deleted JS to the newly added CSS (and adapt where necessary).

3. +++ b/core/modules/quickedit/css/quickedit.theme.css
   @@ -104,10 +104,16 @@
   + * Prevent toolbar from overflowing viewport at narrow screens when
   + * toolbar is in vertical mode.
   
   +++ b/core/themes/stable/css/quickedit/quickedit.theme.css
   @@ -104,10 +104,16 @@
   + * Prevent toolbar from overflowing viewport at narrow screens when
   + * toolbar is in vertical mode.
   

   Supernit: 80 cols…

Rerolled patch from #33.

Updated comments for the min-width, max-width, and general width on the quickedit toolbar container to better explain the reasoning behind why the widths are being implemented this way.

The patch looks good to me, but I think we might need screen shot(s) or an animated GIF to demonstrate the change, if we can't add automated tests. For now, marking as needing screen shots.

Attaching two MP4 movies (better to show the animation). One unpatched and the other patched.

Honestly, I think that looks pretty good. The difference is quite clear, and IMHO it makes Quick Edit feel slicker and more usable. Godspeed!

Thanks for those movies @mherchel. This looks like a good improvement!

#40 thanks for those screencasts — so much better! :)

Thanks everyone! 👏

The fix itself looks like a great improvement. I'm only concerned that the current patch introduces changes that are not required to fix the bug described in the issue summary/title.

1. +++ b/core/modules/quickedit/css/quickedit.theme.css
   @@ -102,9 +102,30 @@
   +  /* Set a minimum width of 240px for the entity toolbar so that
   +  the toolbar never folds up into a squashed and jumbled mess. */
   +  min-width: 240px;
   +
   +  /* Set a maximum width of 450px for the entity toolbar so that
   +  the toolbar doesn't stretch too wide on larger viewports. */
   +  max-width: 450px;
   +
   +  /* Set width of entity toolbar to 100% to ensure the toolbar
   +  fills all the necessary space. */
   +  width: 100%;
   
   +++ b/core/modules/quickedit/js/views/EntityToolbarView.js
   @@ -205,11 +205,6 @@
   -        }).css({
   -          'max-width': document.documentElement.clientWidth < 450 ? document.documentElement.clientWidth : 450,
   -
   -          'min-width': document.documentElement.clientWidth < 240 ? document.documentElement.clientWidth : 240,
   -          'width': '100%'
   

   How are these changes related to the scope of this issue? These change probably could be allowed since the change reduces the weight of these CSS properties instead of increasing them. Anyway, this change should probably be done in its own issue.

2. +++ b/core/themes/stable/css/quickedit/quickedit.theme.css
   @@ -102,9 +102,30 @@
   +/**
   +* Prevent toolbar from overflowing viewport at narrow screens when
   +* toolbar is in vertical mode.
   +*/
   +.toolbar-vertical.toolbar-tray-open .quickedit-toolbar-container {
   +  width: calc(90% - 240px);
   +}
   

   This is a BC breaking change since it affects stable theme. I also don't quite understand how does this relate to the bug described here which is to do with the animations. If this indeed is needed as part of this change, we should add something about the minor BC break to the issue summary.

Bump, lets do this for 8.5 :)

Here's a reroll of #37 that also addresses feedback from #44.

edit: oops, forgot the patch. Plz see next comment :)

Here's a reroll of #37 that also addresses feedback from #44.

tested @jenlamptons patch #50

* patch apply clean
* quick edit is quicker and does not seem nearly as annoying
* concerns from #44 seem to have been addressed properly
* only other changes were about the animation speed 1->0.3, so this patch should only affect what it needs to

+++ b/core/modules/quickedit/css/quickedit.theme.css
index bc5ab75db9..3410fefce7 100644
--- a/core/modules/quickedit/js/views/EntityToolbarView.js

The patch currently only modifies the ES5 version of the JavaScript. The JavaScript changes should be made to the ES6 file and then compiled into this ES5 file.

@narnua thank you for your review and explaining well what you took into account in your review. I updated the commit credit so that you will get credited once the issue lands.

@yoroy thanks for the UX reviews, I also added credit for you.

According to #44, we shouldn't be modifying the JS at all.

The previous patches moved functionality from JS to CSS (which is best practice), but this would necessitate a change to Stable, which is out of scope and breaks BC. We should just be modifying the transition property in the CSS.

Patched attached that changes the timing of the CSS transition and nothing else. This is as basic as it gets :)

Quickedit's animations fall under the new WCAG 2.1 Animation from interactions success criterion.

The related issue to figure out how to support turning animation off moved. We settled on the new prefers-reduced-motion as the simplest approach (currently supported by Safari, and coming soon in Firefox). The WCAG "Animations form interactions" SC is level-AAA, and hence not required by our core accessibility gate. However it looks fairly easy to implement with the media query, and it has a high impact for affected users, so we'd like to address it.

For Quickedit, can we check the media query in JS, before enabling animations? I don't know if that's something we want to do in this issue, or a separate one.

Can't apply patch #54 on 9.2.x .Needs reroll

error: patch failed: core/modules/quickedit/css/quickedit.theme.css:102
error: core/modules/quickedit/css/quickedit.theme.css: patch does not apply
error: patch failed: core/themes/stable/css/quickedit/quickedit.theme.css:102
error: core/themes/stable/css/quickedit/quickedit.theme.css: patch does not apply

Added reroll for patch #54.Please check

Due to Quickedit being moved out of Drupal Core and into a Contrib Module, moving this issue to the Contrib Module queue.