Problem/Motivation

Front-end performance of the toolbar is quite poor in Chrome/safari webkit browsers, reasonable in Firefox and untested in IE(so far). Not in terms of asset loading, but simply in terms of perceived performance on screen.

#1847314: Reduce the dependency on JavaScript for the toolbar to display properly probably helped with that, but when JS is enabled, it's still pretty awful. Very significant flickering. Everything you see in the toolbar, is done through JS, except the bulk of the rendering:

  1. The toolbar is only initialized AFTER the document is ready (because it is implemented as a behavior). Consequently, this has flicker pretty much by design.
  2. Once that is done, even then there are lots of performance problems (too much stuff being called multiple times, too slow jQuery stuff in the critical path, too much JS reliance in general).
  3. Drupal.displace() is called multiple times, and hence can cause multiple displacements of the underlying page. Worse, in order to determine how to displace, it traverses the DOM.

Proposed resolution

Get rid of the useless .each() call (which in case of attaching the behaviors BEFORE the document is ready apparently blocks until the document is ready anyway…)

Remaining tasks

- Review Patch

User interface changes

- No visible changes but we added a hidden control element to toolbar (and which will be removed after JS executed0

(
This is what we have after patching:
- There's should be no vertical jumping anymore
- There's 1px movement from LEFT to RIGHT (affected by active-link.js, it changed the font weight.)
- There's flashing on Toolbar only. This is loading & rendering the SVG icons.
)

API changes

- N / A

Data model changes

- Insert a control element to toolbar elements. (affect theme element only, no DB changes)

Files: 
CommentFileSizeAuthor
#187 People___Drupal_8_x.png327.36 KBplach
#177 core-toolbar-flicker-2542050-177.patch36.13 KBnod_
#157 core-toolbar-flicker-2542050-157.patch27.15 KBdroplet
#154 core-toolbar-flicker-2542050-147.patch27.17 KBdroplet
#151 toolbar_rendering.mp44.19 MBChi
#146 interdiff.patch3 KBdroplet
#146 core-toolbar-flicker-2542050-147.patch27.17 KBdroplet
#141 core-toolbar-flicker-2542050-141.patch25.57 KBnod_
#141 interdiff.txt676 bytesnod_
#139 increment.patch1.55 KBdroplet
#129 2542050-128.core_.Toolbar-implementation-creates-super-annoying-rerendering.patch25.53 KBjonathanshaw
#115 core-toolbar-flicker-2542050-115.patch25.64 KBnod_
#115 interdiff.txt1.45 KBnod_
#113 toolbar-2542050-113.patch24.44 KBdroplet
#113 interdiff.patch16.32 KBdroplet
#111 toolbar-2542050-111.patch20.93 KBSam152
#111 interdiff.txt5.72 KBSam152
#111 toolbar.gif23.12 KBSam152
#102 interdiff.patch2.43 KBdroplet
#102 toolbar-2542050-102.patch15.21 KBdroplet
#92 frontend-bug.png116.79 KBdpacassi
#91 interdiff-2542050-90-91.txt1 KBdpacassi
#91 toolbar-2542050-91.patch13.37 KBdpacassi

Comments

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs work
FileSize
1.63 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,976 pass(es). View
6.06 KB

Here's a few small changes that make the critical path faster (jQuery.hide() is frighteningly slow).

In the "do-not-test" patch you'll find some debug stuff I did. But it still is not at all enough — still significant flicker. The main reason AFAICT is that we only start showing the toolbar in its final place AFTER the entire document has already fully rendered/begun rendering

Wim Leers’s picture

Issue tags: +JavaScript
dawehner’s picture

Thank you for creating an issue wim.

Bojhan’s picture

Title: Toolbar flickers: very bad for UX » Toolbar implementation creates annoying flickering

OMG, yes!

Daniel Schaefer’s picture

Thank you for the patch, Wim.

I just started testing D8 beta and the issue annoyed me so much that I tried various things in DevTools but sadly I didn't have much success. Also your patch did not seem to bring a great improvement.

I wouldn't mind so much if we still had Overlay but since every admin action is a full page load I find this very annoying. I hate to say this, but overall I prefer the admin experience from Drupal 7.

Anyway, I propose to load the nav asynchronously so that we see some kind of throbber until the toolbar is fully loaded. Do you think this could be done with the current architecture?

dawehner’s picture

Title: Toolbar implementation creates annoying flickering » Toolbar implementation creates super annoying flickering

Yeah every user has to deal with that ...

Wim Leers’s picture

+1 to title change. I'm tempted to change it to "EXTREMELY".

Jaesin’s picture

Title: Toolbar implementation creates super annoying flickering » Toolbar implementation creates super annoying re-rendering.

The patch in #1 works and is a huge AX enhancement.

dawehner’s picture

Its a step forward, IMHO, this is not a bad idea.

dawehner’s picture

Status: Needs work » Needs review

.

Wim Leers’s picture

@Jaesin: why is it a huge AX (Authoring Experience?) improvement? In my testing, it barely made a difference :(

droplet’s picture

what kind of flickering? (Screen move down after loaded ? )

effulgentsia’s picture

I typically use Drupal on a wide screen, so have my toolbar to the left rather than at the top. In that configuration, I see my main content moving left and then right every time I navigate to a different page.

Jaesin’s picture

@wim "Administrative Experience".

Exactly what @effulgentsia said. With the toolbar on the left hand side, the page no longer shifts on every page load with the patch from #1.

droplet’s picture

Flicking caused by dimension changes. This patch should fixed the problem (TOP BAR ONLY).

Ideally, it should populate all default class from toolbar module. (and change the default setting from JS side.)

Wim Leers’s picture

+++ b/core/modules/toolbar/css/toolbar.module.css
@@ -21,6 +23,9 @@
+.layout-no-sidebars {
+  padding-top: 79px;
+}

I don't think this is acceptable? This only works for Bartik.

Wim Leers’s picture

#14: and WOOT, yay! Are you sure though? Are you able to consistently reproduce this? E.g. also with simplytest.me? Which browser/OS? Can you verify it in multiple browsers?

droplet’s picture

I don't think this is acceptable? This only works for Bartik.

Unfortunately, this is the only way to deal with this problem AFAIK while D8 don't allow inline scripts. Using CSS3 Animation to make it looks more smoothly is another option (but not perfect IMO)

Themes can set their own value. Client side still calculate and apply new value if it's needed.

If drupal accepts ONE flickring, we can drop fixed CSS value and reduce `change:offsets` calls. (whatever choices, we need to fix it anyway)

Daniel Schaefer’s picture

I made a quick video of it and uploaded to Youtube. FPS was a little low but I think it should be enough to demonstrate the problem.

Drupal 8 - toolbar creates annoying re-rendering

Daniel Schaefer’s picture

@droplet

I tested your patch and must say that it does help my experience a lot!

I have changed a few things in toolbar.module.css though:

body { padding-top: 78px; }
#toolbar-administration { top: 0; }

Your patch is a great improvement to me. Everything stays in place now with the top toolbar and Seven.

droplet’s picture

@Daniel,
Nice!

have some free time to clean up other issues:
#2559769: Use CSS to position toolbar instead JS

Jaesin’s picture

I'm not really seeing the flickering in Beta-16 even without this patch.

bkosborne’s picture

For what it's worth, this problem is not present for me in FireFox (on a Mac), but very apparent on Chrome and Safari.

Wim Leers’s picture

#23: Fascinating! That's very valuable feedback. I wonder if it's because Firefox delays the rendering a bit more, whereas Chrome/Safari are more aggressive in having their first paint happen.

StuartJNCC’s picture

Agree with #22 and #23, I don't see any flickering on Firefox/Windows with the toolbar at the top, but I do see it on Chrome.

nod_’s picture

Mixed the first 2 patches and added some conditions to avoid running some stuff when it's not necessary. It's decent now, no page flicker on page load in chrome and it renders faster.

Some display issues when loading on small screens or changing and closing the trays, but maybe someone will find out what's wrong before me.

Still as fast on Firefox.

nod_’s picture

But it needs to be rewritten to be performant. Tried a few things but backbone gets in the way. The really heavy function is updateTrayOrientation and it's called 4 times during load.

There is just too many indirections in the code, making some way on the rewrite but it'll take a while.

Wim Leers’s picture

Issue tags: +Needs manual testing

Is #26 ready for final review and manual testing, or do you plan more changes?

pwolanin’s picture

Status: Needs review » Needs work

The arrow to make the toolbar vertical seems to be missing with this patch.

Also, the flash is less, but still visible for me in chrome.

tarekdj’s picture

FileSize
387.83 KB

The arrow to make the toolbar vertical seems to be missing with this patch.

Confirmed!
Also, the vertical toolbar breaks on resizing. See GIF attached.

droplet’s picture

It's PHP + JS libs, not a purely JS lib. the Black toolbar main structures already rendering with PHP code (I've surprised. I was thought it's all from ajax requests) . Actually at this point we can pre-render all menu with styling. and then JS checking for further changes.

It's super simple but seems like this way banned for some reason. It's what I don't understand.

OnkelTem’s picture

Hi!

Installed D8 for the first time only yesterday and seems like I faced exactly what the TS reports. See this video: https://youtu.be/urZ02CLzrAU

andrewmacpherson’s picture

Version: 8.0.x-dev » 8.1.x-dev
sf_wind’s picture

Oh really? We are not going to fix it in 8.0.x? In my opinion, it is not usable...

nod_’s picture

Version: 8.1.x-dev » 8.0.x-dev

Still up for 8.0.x, no api change, just optimization.

lokapujya’s picture

Can the "back to site", and the displace of the the dropdown menu be rendered without javascript?

joewebmaster’s picture

I am just learning Drupal 8 and I'm seeing this same annoying re-rendering issue with the Drupal 8 Admin Toolbar. And I agree, it's super annoying.

Since I'm not to the skill level where I've learned how to apply one of these patches yet, I'm wondering if this is something that would be "fixed" in a future release of Drupal 8? Excuse my ignorance on how these patches and fixes work, and/or how they are eventually implemented/approved to be applied to a Drupal release.

I ran across this thread while searching for a fix on Google, and it appears that this conversation is among high-level Drupal developers, so maybe I shouldn't even be posting here, but I am about to pull my hair out, trying to figure out how to get rid of the annoying toolbar issue, so hopefully someone can take mercy on me and tell me if they think a fix will be coming soon?

Wim Leers’s picture

I'm wondering if this is something that would be "fixed" in a future release of Drupal 8?

Absolutely.

hopefully someone can take mercy on me and tell me if they think a fix will be coming soon?

Currently nobody is working on a fix, so the honest answer is: no idea when, for now it doesn't look like it'll be any time soon, precisely because nobody is working on it — but, of course, feel free to help fixing it!.

droplet’s picture

@joewebmaster ,
Check it out!

I wasn't plan to release it before. Use at your own risky (pretty safe to use I can say)

https://www.drupal.org/project/toolbar_anti_flicker

(It's only fixing for horizontal only for 80% users I care. I have no plan to support vertical version now)

Wim Leers’s picture

That module only fixes it in the most narrow use case: with the default theme, default toolbar items, etc. Also, it loads that additional CSS and JS on all pages, and for all users, so it causes worse front-end performance.

As a JavaScript maintainer for Drupal 8, why not help fix the problem in a generic way? (It confuses me that you would propose this hacky module as a solution.)

Daniel Schaefer’s picture

@droplet I gave your module a go and it does feel a bit better now. The difference is not very big for me though (could be because I've already applied the patches). But at least better than the original. Most importantly, the content below the toolbar doesn't jump around anymore. I believe that was the most annoying issue for users.

I do agree with Wim though about a more generic solution. That would benefit more users. droplet's module could be a temporary fix for those who can't wait.

How does everyone feel about reintroducing some kind of overlay? I'm well aware of the discussion about the old solution but actually I've had a better experience with Overlay in 7. There would be no need for re-rendering the toolbar on every page. It's a shame it had to be removed.

droplet’s picture

@Wim Leers,

Although the hacking workaround provided better experience IMO, I don't think the CORE will accept it now.
(the problem mentioned in #16, I have no idea how to sorted it perfectly)

The another good solution for CORE I have in mind is rewrite all CSS in toolbar (#31) . It's also not possible for D8.0 version ??
(and it will be fixed horizontal only. No good workaround for vertical version)

For D8.0, the remaining issue is:

A. Agree to accept fixed "padding-top: 79px;" in CORE ( It may possible to expose a API for theme to change it...etc )
B. Or agree to accept bloody CSS changes

A is better than B IMO.

@Daniel Schaefer,

Make sure you use 1.1 version, I made a mistake on module name before.

Daniel Schaefer’s picture

Yep, used 1.1 thanks. Also ran a test on simplytest.me which confirmed the improvement.

I will sit down over this and see where I may be able to contribute. Sadly I'm not involved in a lot of web development atm so time is a bit limited for all this.

Wim Leers’s picture

We can completely change how the Toolbar is rendered in 8.1 to fix this very nasty UX problem.

joewebmaster’s picture

@droplet Thanks so much. I will give it a try! One thing else, that someone had also mentioned previously - this problem only occurs for me when using Chrome and Windows Edge. It works completely fine when I'm using Firefox.

droplet’s picture

*** My bloody code only demonstrate the changes it needed to fix flickering for horizontal toolbar. ( Absolutely it needed to tidy up coding) ***

My eye is so tired. Needs Visual reviews from you :)

- It's fixed body jumping
- Toolbar itself still flickering ( don't let it disturbing your eyes, try hide-toolbar.patch )

-------------

** You can skip to review the JS code.

Main changes:

+++ b/core/modules/toolbar/toolbar.module
@@ -356,3 +356,17 @@ function _toolbar_get_subtrees_hash() {
+function toolbar_preprocess_html(&$variables) {
+  if (!\Drupal::currentUser()->hasPermission('access toolbar')) {
+    return;
+  }
+  $attributes = array();
+  $attributes['class'] = 'toolbar-tray-open toolbar-horizontal';
+  $variables['attributes'] = new Attribute($attributes);
+}

Pre-render the class

I think this version is possible get into D8.0. :)

droplet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +CSS
FileSize
9.09 KB

WOW. Can't believe I did it.

Due to the complexity of this issue, I can't describe my code in word to you. Please review it :) I hope code explained itself.

Testing Notes:
- ** CLEAR ALL CACHES ** AFTER APPLIED PATCH
- don't test in page with FORM, it may caused extra flashing from FORMs.
- Switching between 2 pages both with scroll bar ( or both without scroll bar ).
- testing in remote or throttling network you can get better results

Just would like to highlight one point:

+++ b/core/modules/toolbar/toolbar.module
@@ -205,6 +205,12 @@ function toolbar_toolbar() {
+  // Cloned as control element to aviod toolbar flickering issue.
+  $items['shadow'] = $items['administration'];
+  $items['shadow']['#wrapper_attributes']['class'] = array('invisible');
+  // Place as last item.
+  $items['shadow']['#weight'] = 999;
+  $hash_cacheability->applyTo($items['shadow']);

Cloned one toolbar item as control element and make it as "position:relative" item to help to calc the height value on "position:absolute".

@see #54

Pre-FAQ:
- Why don't set toolbar as "position:absolute/fixed" until scroll event ?
It sorted the last flickering in toolbar. Compare patch#46, you can find out the answer.

Known issue:
- Vertical toolbar do not fixable (unless we save the state in DB)
- extra: hoping that someone could clean up the CSS in toolbar in D8.1 :)

Status: Needs review » Needs work

The last submitted patch, 47: toolbar-perfect-fix.patch, failed testing.

joelpittet’s picture

Issue summary: View changes

Yeah vertical toolbar is a bit worse with that patch because it goes into horizontal mode for a second then jumps up and then left after it renders. This patch does seem to make horizontal mode not jump.

Also an FYI, this is only a problem on Chrome/webkit, Firefox doesn't jump in either mode it seems as mentioned a few times above so adding to issue summary and removed a bit of the snark.

droplet’s picture

Yeah vertical toolbar is a bit worse with that patch because it goes into horizontal mode for a second then jumps up

Either from left to right or bottom left to top right motion. my eyes can't read the words or perform some actions with mouse..etc. (we may add transition there to make users feeling better.) As a tester we may feel bad. But in reality (vertical mode) users will get used to these behaviors quickly.

It needed some trade-off and I believe the vertical only make for mobile. Users will make a clear decision after the patch.

Anyway if we believed the word @Wim Leers made in this comment, it's great for many users with slow network.
https://www.drupal.org/node/2645666#comment-10751452

droplet’s picture

Status: Needs work » Needs review
FileSize
9.2 KB

Fix tests

Status: Needs review » Needs work

The last submitted patch, 51: toolbar-perfect-fix-51.patch, failed testing.

droplet’s picture

Ahh. Actually we didn't need the control item.

droplet’s picture

Status: Needs work » Needs review

The last submitted patch, 46: toolbar.patch, failed testing.

Daniel Schaefer’s picture

I've applied the patch to 8.0.2 on simplytest.me. The experience in Chrome is great. Everything stays in place with the horizontal toolbar. The vertical toolbar does jump a little bit though on page loads.

I'd say it is a good improvement overall, although it seems there is another bug:

  1. Toolbar bottom tray not hidden properly

    This problem is caused by the display property in line 80 of toolbar.module.css:

    .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray {
        position: relative;
        display: block; /* prevents bottom tray from disappearing */
        z-index: -999;
    } 
    

    When you press a top tray button twice the bottom tray is supposed to disappear. The height is being set correctly however the display: block property overwrites display: none in line 156. The fix is easy:

    Change .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray {
    to .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray.is-active {

    I've tested the fix in Chrome and it didn't break anything.

  2. Another strange thing happened with the user menu (I'm not sure if it had something to do with the patch)

    Vertical toolbar issue

    Problem here are the position: absolute and position: relative properties in line 85 and 80 of toolbar.module.css which prevent the position: fixed property in .toolbar .toolbar-tray-vertical.is-active, body.toolbar-fixed .toolbar .toolbar-tray-vertical (203) to apply.

    I know it looks a bit dirty but adding !important to the position: fixed in 203 did the trick.

    toolbar.module.css (line 203)

    .toolbar .toolbar-tray-vertical.is-active, body.toolbar-fixed .toolbar .toolbar-tray-vertical {
        height: 100%;
        overflow-x: hidden;
        overflow-y: auto;
        position: fixed !important; /* fix the broken user menu */
    

    After that it seems fine:

    Vertical toolbar issue

    I really like the patch and we should commit it to core! Although some more testing with different themes enabled and in different browsers wouldn't hurt.

Daniel Schaefer’s picture

FileSize
5.88 KB
7.32 KB
8.48 KB

I've noticed another thing. The responsive behavior is really strange.


  1. Responsive problems

  2. Responsive problems

  3. Responsive problems

I'm out of time for now but I'll try to have a deeper look into it later. If anyone likes to investigate, an easy way to get started is simplytest.me. Just add the latest patch there and you are ready to go.

EDIT.

Ok, looks like adding position: absolute to .toolbar-tab makes the black space go away but breaks the desktop view. And the user icon is still out of place due to positioning. Probably need a few media queries here.

Daniel Schaefer’s picture

Status: Needs review » Needs work
droplet’s picture

@Daniel,

Thanks!

https://www.drupal.org/files/issues/20160119-163445_capture.gif
is it using bootstrap theme?

It reminded me we may get back to use padding-top and should be applied to html tag to avoid any side effect from custom theme..etc

Other problems (and more further fixing & clean up) I've addressed in my local patch already. Sorry for the late to upload. It needs some more time to clean it up before public testing.

Daniel Schaefer’s picture

Hi Droplet, I'm planning to spend some time on a D8 project this weekend so just checking if #53 is the latest patch available? I'm ready to do some testing if you provide a new patch.

droplet’s picture

Status: Needs work » Needs review
FileSize
10.82 KB

** GIT RESET --HARD && APPLY PATCH && CLEAR ALL CACHES **

My Final Conclusion for this issue:
- It can be only solve from CSS side.
- Vertical toolbar is not fixable. (unless we introduce options in user profile section to pre-populate CSS class)

To Reviewers:
- It's a complicated issue. Please upload your patch with fixes. It's the only way to help us understand the whole picture.
- Real testing before posting questions.
- Testing in CORE THEME FIRST, then custom theme. If you find any problem with custom theme, upload your theme / links. For example, bootstrap. They add extra marge/padding to top cause the jumpy. Custom theme should fix it themselves. Make custom compatible to CORE, not Core following contributions.
- Please also including the testing steps (also the path, eg: admin/structure) if you find a bug.

To Patcher:
- You shouldn't trust any code comments in all Toolbar JS & CSS files.

To anyone needed an interdiff:
- Sorry, no interdiff is provided. Here's not a typo fixing. You should not review code from interdiff.

My Final Conclusion for toolbar:
- We can drop Backbone.
- Redesign whole toolbar, including UI & Features.
- We shouldn't accept all suggestions and feature requests. (Personally, I did a quick testing and end up a lightweight version with basic features with 50 lines for JS & 70 lines for CSS.)

This is my last attempt for this issue. If anyone have time, please take over my work.

Thanks ALL!

droplet’s picture

Just read in other thread, there's only 2 more day before 8.1.0 beta.

Daniel Schaefer’s picture

FileSize
57.11 KB

Just ran your latest patch on a fresh simplytest.me 8.0.4. Toolbar becomes unusable (screenshot attached). #53 is working good except the glitches described in #56 and #57. Issue with the patch file?!

Patch 61 applied to clean Drupal 8.0.4

droplet’s picture

Ahh. Missing stable diff in #61. So this patch just cloned the changes to Stable theme and git diff between 2 branches.

EDIT: I ran a simpletest, that's fine now.

geerlingguy’s picture

Attached a video of the behavior (with the patch) in Chrome/stable latest on Mac OS X. The jumpiness is minimized slightly, but still present enough to be jarring to me (it just makes every page load feel a little funny). But it _is_ reduced as a result of this patch; maybe 1-2 frames faster on my MacBook Air.

lokapujya’s picture

FileSize
900 bytes

I had this idea to delay painting of the toolbar until it's completely rendered, but this is perhaps too drastic of an approach. Although I do think this is sort of what we need to do, but preferably in a more Backbone like way.

EDIT:

It doesn't work because still a jump after loaded.

But, it can be modified to repaint after the jump.

droplet’s picture

@geerlingguy,

It's not what I expected. It should be very smooth without any flashing, jumping or flickering. Make sure no caching after patch is applied.

@lokapujya,
It doesn't work because still a jump after loaded.

geerlingguy’s picture

@droplet - After I applied the patch I did a drush cr then refreshed the page a few times; is there anything further required? I'll also test on simplytest.me.

[Edit: works great on simplytest.me... must be a local environment issue. Weird.]

iampuma’s picture

Patch #64 works nicely for me. At least I do not have that flickering annoyance anymore under my skin.

ParisLiakos’s picture

yes, #64 works now, at least when it is on horizontal mode.
It stil jumps on vertical mode and when its completely hidden.
In any case, its an improvement

+++ b/core/modules/toolbar/css/toolbar.theme.css
@@ -93,8 +93,7 @@
-.toolbar-tray a.is-active
- {
+.toolbar-tray a.is-active {

+++ b/core/modules/toolbar/js/models/ToolbarModel.js
@@ -3,7 +3,7 @@
-(function (Backbone, Drupal) {
+(function ($, Backbone, Drupal) {

@@ -154,4 +154,4 @@
-}(Backbone, Drupal));
+}(jQuery, Backbone, Drupal));

+++ b/core/themes/stable/css/toolbar/toolbar.theme.css
@@ -93,8 +93,7 @@
-.toolbar-tray a.is-active
- {
+.toolbar-tray a.is-active {

Unrelated changes

droplet’s picture

@ParisLiakos,

It stil jumps on vertical mode and when its completely hidden.

It's known bugs. We can only making it work for 80%

Getting feedback for perfect solution:
#2696023: Save Users' Toolbar State config to serverside

Unrelated changes

We can dropped the CSS changes. But it's not a big deal here. :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpacassi’s picture

Hey guys!

I applied the patch provided in #64 but it broke the IMCE module (because it is overwriting the $variables['attributes']['class'] variable instead of just appending the new classes), so I came up with a new patch.

Feel free to test it and give feedback.

Cheers!
David

Status: Needs review » Needs work

The last submitted patch, 73: toolbar-reworked-perfect-fix-2542050-72.patch, failed testing.

dpacassi’s picture

Had an error in my last patch, please test this one.

droplet’s picture

@dpacassi,

I see your points. I just fixed the same thing in #2707675: preprocess_html remove attributes. Can you also review #64 fully? Then we able to move into next point.

Thanks.

andrewmacpherson’s picture

Status: Needs work » Needs review
FileSize
14.17 KB
4.05 KB

The patch in #75 has some new functions with odd names, looks like r/l typo.

+    updateToolbalHeight: function () {
+    setToolbalHeight: function () {

Re-rolled patch changes these names to:
updateToolbarHeight
setToolbarHeight

dagmar’s picture

+++ b/core/modules/toolbar/toolbar.module
@@ -356,3 +356,20 @@ function _toolbar_get_subtrees_hash() {
+function toolbar_preprocess_html(&$variables) {

This should include a docblock like this:

/**
*Implements hook_preprocess_HOOK() for HTML document templates.
*/

dpacassi’s picture

Good eyes @andrewmacpherson !
I only added the

  $attributes = isset($variables['attributes']) ? $variables['attributes'] : array();
  if (!isset($attributes['class'])) {
    $attributes['class'] = array();
  }

  $attributes['class'][] = 'toolbar-tray-open';
  $attributes['class'][] = 'toolbar-horizontal';
  $attributes['class'][] = 'toolbar-loading';

part to the patch #64, I wasn't aware of any typos existing in that patch.

@droplet & @dagmar:
I've included the docblock in the same style as in the rdf or node module.
Also, I've made some minor code styling updates.

@all:
The new patch was built on top of #77 and is ready for testing.

droplet’s picture

droplet’s picture

jibran’s picture

Issue tags: +JavaScriptTest

@Wim Leers should we add functional-javascript tests for this?

droplet’s picture

Ouch #81 has wrong code again. Is Windows Bash has cache problem? But the interdiff is correct :s

@jibran,
I think no one would say no (especially in Drupal community). but when and what we should cover in THIS issue is a question.

jonathanshaw’s picture

Given how "super annoying" this is, delaying the patch for complex new tests would seem to be more harm than good. Start building toolbar tests in a follow-up?

droplet’s picture

I'm planning to release a module with single line top bar + verticle mode. (or V mode only) It's also has a opened verticle menu in backend by default.

In second toolbar menu, `Content` is the only menu item for most site editors. A simple dropdown or fixed V mode for other menus would save all developers. or a second click to backend for extra admin actions.

If we all love it, we can be game changer again.

We should design developers tools for developers, not a group of learner at UX labs.

lokapujya’s picture

Due to the complexity of this issue, I can't describe my code in word to you.

I think it would be helpful if you could try to describe the architecture change in this patch.

droplet’s picture

@lokapujya,

JS Part:

+++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
@@ -45,9 +45,15 @@
+      this.listenTo(this.model, 'change:activeTab  change:orientation change:isOriented change:isTrayToggleVisible', this.render);
...
+      this.listenTo(this.model, 'change:activeTab change:orientation change:isOriented', this.updateToolbarHeight);
+      this.listenTo(this.model, 'change:height', this.setToolbarHeight);

We didn't handle everything in this.render() anymore. Just like in PHP, you won't HOOK everything in same API function.

NOW the toolbar render in this way: (not exalty this order)
RENDER -> TOGGLE many classes -> SET PADDING -> TOOGLE TOOLBAR ON -> TOGGLE many classes -> TOOLGLE OFF -> HOME BUTTON -> Toogle many stuff -> Final

each EVENT will go through all these steps

If that's fast enough (say within 16ms), you may not see the SECOND EXTRA jumpy (but with flashing).

Patch:
Reduce the changes. (This isn't ideal states, but I'm scared to make changes in the same patch)

CSS:
the changes in CSS trying to reduce the FIRST JUMP.
(turn off JS in browser to understand more)

droplet’s picture

@All,

I pushed a big update to my module, take a look if you want to test it without hacking CORE.
https://www.drupal.org/project/toolbar_anti_flicker

Wild Test & Rapid Release Makes Perfect.

andypost’s picture

+++ b/core/modules/toolbar/css/toolbar.theme.css
@@ -93,8 +93,7 @@
-.toolbar-tray a.is-active
- {
+.toolbar-tray a.is-active {

+++ b/core/modules/toolbar/js/models/ToolbarModel.js
@@ -3,7 +3,7 @@
-(function (Backbone, Drupal) {
+(function ($, Backbone, Drupal) {

@@ -154,4 +154,4 @@
-}(Backbone, Drupal));
+}(jQuery, Backbone, Drupal));

+++ b/core/themes/stable/css/toolbar/toolbar.theme.css
@@ -93,8 +93,7 @@
-.toolbar-tray a.is-active
- {
+.toolbar-tray a.is-active {

looks out of scope but ++

droplet’s picture

Thanks @ALL,

- Removed unnecessary changes.
- Tidy up Code Style on my changes.
- Merged back few changes from ( https://www.drupal.org/project/toolbar_anti_flicker ).

Remind again: CLEAR CACHES & CLEAR CACHES ON PATCH TESTING :)

dpacassi’s picture

@droplet: I applied your patch to my site, looks great so far! Thanks for the job!

I saw that you missed a blank between a "left:" and its value "0" in two CSS files.
I created a new patch for that and also an interdiff from your patch to mine.

dpacassi’s picture

FileSize
116.79 KB

@droplet: I actually had to go back to patch #79.
I've noticed some strange behavior with the latest patch on Firefox 45.0.2 on Mac OSX.

When loading admin pages, the menu would still get built for something like half a second, then the toolbar looks like this:
Frontend bug

I guess we'll need to investigate this further :-/

droplet’s picture

@dpacassi,

It looks like you're using one of "Admin Toolbar" with extra sub dropdowns. For example, I covered "Admin Toolbar" in my module but didn't add to CORE because I think it's a contributed module issue more.

http://cgit.drupalcode.org/toolbar_anti_flicker/tree/css/toolbar-anti-fl...

+++ b/core/modules/toolbar/src/Element/ToolbarItem.php
@@ -77,7 +77,6 @@ public static function preRenderToolbarItem($element) {
-      $element['tray']['#wrapper_attributes']['class'][] = 'toolbar-tray-horizontal';

+++ b/core/themes/stable/css/toolbar/toolbar.module.css
@@ -78,17 +78,17 @@
+  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray {
...
+  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray .toolbar-lining {
...
+  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .home-toolbar-tab + .toolbar-tab .toolbar-tray {

These few changes made the diff.

I trying not to hack standard ToolbarItem and still have little hope of adding vertical mode supports to CORE. :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

paulwdru’s picture

Surprisingly this issue had not been resolved in core in Drupal 8.23

Many laymen, newbies & testers were turned away by this flicker which seriously spoiled the image of Drupal in terms of user experience as well as confidence

Joomla & Wordpress have long been famous of their UI which look friendly, trendy & striking in first sight. Drupal 8 UI was redesigned to compete with them but unfortunately spoiled by this flicker

Chi’s picture

@paulwdru, I totally agree, but to move this forward we need someone to spend time on reviewing the patch. :)

Personally, I install Toolbar anti flicker module on each Drupal 8 installation.

paulwdru’s picture

@Chi,

I asked my friends to test Drupal 8 for their new projects but they just uninstalled Drupal after playing around for 5 minutes due to this Flicker and said feeling fainted with it, no confidence.

First impression is very important especially for newbies to Drupal as we CANNOT expect them to look for modules to patch the core interface right after installation. Similarly, we won't expect that we need to patch our new cars before driving.

Why don't we commit Toolbar Anti Flicker module into core first instead of letting this Flicker spoil the image of Drupal ? There are many Drupal 8 tutorial videos on Youtube showing this Flicker to the world, sorry to say, what an eyesore.

Just my 2 cents, thanks

lokapujya’s picture

My idea in #66 might have been before I fully understood the problem: Would #66 work if the repaint was moved into the end of the setTimeout()? It might not be fixing the real problem and I couldn't make it work when I tried it 8 months ago.

Let me describe the #66 patch it in more detail:
Copy the toolbar.
Show the copy of the toolbar.
Make all changes to the toolbar.
Replace the Copy with the new toolbar.

An issue with this fix might be that you are supposed to see animations and that this would hide animations.

droplet’s picture

I have no idea too. Also #2696023: Save Users' Toolbar State config to serverside.

@lokapujya,

I do not fully understand your last comment. But by reading your patch. It doesn't work. Extra repaint caused by JavaScript is another problem we have to faced. BUT to get rid of all JS, we still facing the CSS repaint.

Googling any of this keyword may help you to understand to a problem:
FOUT
FOIT
FOFT

(searching "webfont" with above keywords, It's easier to understand the problem)

there're few diff level we need to reached:

- No jumping: my patch (extra plus: #2696023: Save Users' Toolbar State config to serverside)
- less jumping: (@Wim Leers or @lokapujya. their patches may not work but I think their core idea would be that way)
- anime: (OutsideIn like anime. Ping OutsideIn team, Dries may deal with it himself, lol)

I think me & most of the real world users only accept my patch way.

(To myself, it still not the perfect solution. Drupal doesn't use a React-like framework. We have a lot of toolbar code pre-rendered in HTML already. It needs not such complicated JS logic code.)

lokapujya’s picture

Thanks. So, just to help people out, is #91 the patch that should be reviewed right now?

droplet’s picture

@lokapujya,

Yes, #91 is the final patch.

droplet’s picture

Add back the only change from the module:
https://www.drupal.org/node/2779317

So far, toolbar_anti_flicker module has no new bug report about this Flicker bug after August 5, 2016.

droplet’s picture

Status: Needs review » Needs work

A new patch is coming soon.

paulwdru’s picture

Great to see the patch to be committed in core soon. Often we see many new backend features / enhancements being added instead of patching the frontend as of top priority.

Hopefully Drupal will prioritize patching the frontend namely #2729575: Drupal 8 is just Half-baked Responsive if compared to BackdropCMS

Bojhan’s picture

Assigned: Unassigned » Wim Leers

@Wim Could you take a look at this?

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

@Bojhan: I'm not a maintainer of the Toolbar module. @nod_ is. Still, reviewed it anyway because you asked.

  1. +++ b/core/modules/toolbar/css/toolbar.module.css
    @@ -56,18 +56,43 @@
    +  left: 0;
    ...
    +  right: 0;
    

    Needs LTR support

  2. +++ b/core/modules/toolbar/css/toolbar.module.css
    @@ -56,18 +56,43 @@
    +@media (min-width:61em) {
    

    What's the origin of this dimension?

  3. +++ b/core/modules/toolbar/css/toolbar.module.css
    @@ -258,3 +280,13 @@ body.toolbar-tray-open.toolbar-vertical.toolbar-fixed {
    +/**
    + * Toolbar home button toggle.
    + */
    +.toolbar .toolbar-bar .home-toolbar-tab {
    +  display: none;
    +}
    +.path-admin .toolbar-bar .home-toolbar-tab {
    +  display: block;
    +}
    
    1. Is this change really necessary too?
    2. The comment is wrong; this is not about styling that toggle, it's about optimizing the speed/avoiding flickering of it.
    3. The first CSS rule ensures it's hidden by default, the second ensures it's shown by default on /admin/* paths, right?
  4. +++ b/core/modules/toolbar/js/views/BodyVisualView.js
    @@ -25,28 +25,15 @@
         render: function () {
           var $body = $('body');
    -      var orientation = this.model.get('orientation');
    -      var isOriented = this.model.get('isOriented');
           var isViewportOverflowConstrained = this.model.get('isViewportOverflowConstrained');
     
           $body
    -        // We are using JavaScript to control media-query handling for two
    -        // reasons: (1) Using JavaScript let's us leverage the breakpoint
    -        // configurations and (2) the CSS is really complex if we try to hide
    -        // some styling from browsers that don't understand CSS media queries.
    -        // If we drive the CSS from classes added through JavaScript,
    -        // then the CSS becomes simpler and more robust.
    -        .toggleClass('toolbar-vertical', (orientation === 'vertical'))
    -        .toggleClass('toolbar-horizontal', (isOriented && orientation === 'horizontal'))
             // When the toolbar is fixed, it will not scroll with page scrolling.
             .toggleClass('toolbar-fixed', (isViewportOverflowConstrained || this.model.get('isFixed')))
             // Toggle the toolbar-tray-open class on the body element. The class is
             // applied when a toolbar tray is active. Padding might be applied to
             // the body element to prevent the tray from overlapping content.
    -        .toggleClass('toolbar-tray-open', !!this.model.get('activeTray'))
    -        // Apply padding to the top of the body to offset the placement of the
    -        // toolbar bar element.
    -        .css('padding-top', this.model.get('offsets').top);
    +        .toggleClass('toolbar-tray-open', !!this.model.get('activeTray'));
    

    Woah this is a massive change. Does this mean you can no longer switch between horizontal & vertical orientation?

  5. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -45,9 +45,15 @@
    +      // Put it at top to aviod extra reflow.
    

    Nit: s/aviod/avoid/

  6. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -45,9 +45,15 @@
    +      this.listenTo(this.model, 'change:activeTab  change:orientation change:isOriented change:isTrayToggleVisible', this.render);
    

    Nit: two spaces after activeTab.

  7. +++ b/core/modules/toolbar/toolbar.module
    @@ -277,6 +277,19 @@ function toolbar_menu_navigation_links(array $tree) {
    +  $variables['attributes'] = new Attribute(array(
    

    Nit: s/array()/[]/

  8. +++ b/core/themes/stable/css/toolbar/toolbar.module.css
    @@ -56,18 +56,43 @@
    +.toolbar .toolbar-tray {
    +  position: absolute;
    +  width: 100%;
    +  left: 0;
    +}
    

    This does NOT match what's in core/modules/toolbar/css/toolbar.module.css.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

I have an inferior but less invasive approach here #2850708: Alternative toolbar flicker solution.

Love the result of this patch though. The hardcoded 63em does seem like an issue, that's cool for contrib but in core people would expect that it follow their breakpoint settings.

droplet’s picture

The patch here is not hardcoded. It will take theme's styling. But it will assume horizontal first (on large screen). (Appling 80 / 20 rules)

Also, I have a new patch already but a bit lazy and no hope and don't want to make more noise before more clear thoughts. I restored it back to padding in my new patch. (fixing the content preview problem I didn't notice before)

#2850708: Alternative toolbar flicker solution and no patch almost same but with slightly diff visual effect. #2850708: Alternative toolbar flicker solution looks better in visual. It's similar to with anime. (e.g. in Setting Tray)

However, it still has "movement", a jumping. That's the most annoying problem. I will take #2850708: Alternative toolbar flicker solution (tool-**.js part) as performance improvement, not a solution.

EDITED.

nod_’s picture

I got some time these days. I can take care of the nitpicks if you upload your updated patch :)

As I'm starting to work on more D8 sites it's starting to get on my nerves.

Sam152’s picture

Status: Needs work » Needs review
FileSize
23.12 KB
5.72 KB
20.93 KB

This looks awesome. One thing I noticed taking it for a spin was a slight flicker of a different kind:

Perhaps one approach to fixing it would be to add the loading class to the parts of the theme concerned with the horizontal layout that it defaults to while loading.

Edit: I changed branches back to 8.4.x and now I'm sad again :(

Sam152’s picture

Status: Needs review » Needs work

NW for #106.

droplet’s picture

Status: Needs work » Needs review
FileSize
16.32 KB
24.44 KB

- This is more optimized.
- I re-implemented @Sam152 changes. The remaining 1px move comes from font-weight of active tab changes.

Just drop a breakpoint in ToolbarVisualView and compare the differences if you're interested why I patch it that way :p You will find there's still some more room can be improved.

nod_’s picture

Oh wow, that's awesome! No flicker anymore

Basically switching the rendering is switched to desktop-first. Does break the mobile experience though. Tried to poke around but can't reliably improve the experience.

We need to fix the mobile exp and we're good to go :) I'll close my issue.

nod_’s picture

Status: Needs review » Needs work

The last submitted patch, 115: core-toolbar-flicker-2542050-115.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

green

droplet’s picture

Ah right. I almost forget the mobile testing.

Looking at #115 fixes, it may be a bug of the toolbar itself. Toolbar Views don't respond to ToolbarModel default config. I've done some little fixes in #113.

If it's not a bug, I think we can fix the mobile exp from CSS side.

(I will take a look at coming weekend but no promises, hehe. Thanks All)

nod_’s picture

The media queries are executed before the toolbar views are bound to the model, that's why nothing is done. Can't be a css-only fix because 2 classes need to be removed for the toolbar to work fine.

If the CSS people are happy with the patch, nothing stopping us

droplet’s picture

@nod_, makes sense. Any reviewers?

lauriii’s picture

Status: Needs review » Needs work

I reviewed the CSS and for the most part it looks fine. Probably just a few changes that could be made to make it easier to read if the CSS is to be changed later.

  1. +++ b/core/modules/toolbar/css/toolbar.module.css
    @@ -56,18 +58,43 @@
    +/* Flickering fix */
    
    @@ -85,13 +112,22 @@ body.toolbar-tray-open.toolbar-fixed.toolbar-vertical .toolbar-oriented {
    +/* Flickering fix */
    

    It would be useful to have better documentation about what happens here. It is quite a few classes that are involved and it took me quite long to figure out this.

  2. +++ b/core/modules/toolbar/css/toolbar.module.css
    @@ -56,18 +58,43 @@
    +  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray {
    ...
    +  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .toolbar-tab:last-child .toolbar-tray .toolbar-lining {
    ...
    +  .toolbar-loading.toolbar-horizontal .toolbar .toolbar-bar .home-toolbar-tab + .toolbar-tab .toolbar-tray {
    

    Do these selectors actually need the .toolbar class?

droplet’s picture

Will it work:
// .toolbar-loading is required by Toolbar JavaScript to pre-render markup style to avoid extra reflow & flicker.

Do these selectors actually need the .toolbar class?

Depends.

1. Personally, I preferred to remove it.
2. Considered in safety, Keep it. If anyone cloned the code style from current Toolbar's CSS as I do in the patch. It really has a chance another rule's from contrib, the CSS specificity is higher than patch's CSS between the JS is running. (BUT, I don't want to drop a !important there.)

Many Toolbar's CSS coded like this:
.toolbar .toolbar-bar .toolbar-tab

To me, I will do this:
.toolbar .toolbar-tab

Even this instead:

.toolbar-tab /** .toolbar__tab **/
nod_’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
25.87 KB

Added comment. Didn't change selectors with the .toolbar class.

Not sure what the second flickering fix comment should say so I left it alone.

droplet’s picture

+++ b/core/modules/toolbar/css/toolbar.module.css
@@ -85,13 +114,22 @@ body.toolbar-tray-open.toolbar-fixed.toolbar-vertical .toolbar-oriented {
+body.toolbar-tray-open.toolbar-fixed.toolbar-vertical #toolbar-administration {
+  margin-left: -240px;
+  margin-left: -15rem;
+}

This is used by my module to fix the vertical movement and introduced from #113 accidently. We can remove it from CORE patch.

Generally speaking,

+++ b/core/themes/stable/css/toolbar/toolbar.theme.css
@@ -74,11 +74,11 @@
-.toolbar .toolbar-tray-horizontal {
+.toolbar-horizontal .toolbar-tray {

All these changes are clean-up more than fixing for flicker, even it does.

Theoretically,
1. #toolbar-administration is the ROOT of toolbar component;
2. if it's flickering at day one. We should not put these classes (toolbar-tray-open toolbar-horizontal) in BODY. Even now, .toolbar-tray-open should not there;
3. and most of the dimension & position styles should move to #toolbar-administration;
4. and only ONE horizontal/vertical class to flag the toolbar state in #toolbar-administration;
5. .toolbar-oriented, I don't know what that is even it's documented. When we have horizontal & vertical classes, why create another class?
6. more ..
7. I'm not trying to blame anyone :)

Drupal Core needed a man or two to scan all CSS STYLE to keep it with better consistency. Just following a loose code style is not enough. And it's good to do it with a hidden guy/way, no pressures to say F-word. :)

lauriii’s picture

Status: Needs review » Needs work

Fair point. Let's keep the selectors as is now and do the clean-up in another issue.

+++ b/core/modules/toolbar/css/toolbar.module.css
@@ -78,7 +78,9 @@
+/* Flickering fix
+ * .toolbar-loading is required by toolbar JavaScript to pre-render markup
+ * style to avoid extra reflow & flicker. */

According to our CSS formatting guidelines, this should follow doxygen comment style. More info: https://www.drupal.org/docs/develop/standards/css/css-formatting-guidelines

This is used by my module to fix the vertical movement and introduced from #113 accidently. We can remove it from CORE patch.

Let's do so.

nod_’s picture

Fixed the comment so that it's consistent with what's already in that file.

Removed the extra css rule core doesn't need. Still no flicker :)

tameeshb’s picture

Few more comment style fixes from toolbar.module.css.
Please review

droplet’s picture

#126,

Code changes look fine to me. Styling checking leave to another reviewer :)

#127,

This is over-patched I think. Many of comments' style errors are not introduced by the current patch. That should fix in another issue thread.

jonathanshaw’s picture

Reuploaded patch from #126 for clarity.
All points identified have been fixed as far as I can see.
Sign offs from theme (#125) and javascript (#126) maintainers obtained.

I still get flicker on Windows Edge, but it's not worse than it was before, and is so much better in other browsers that I suggest we don't hold the patch up for that. It can be a follow-up if necessary..

droplet’s picture

Issue summary: View changes

@jonathanshaw,

Remember to clean cache after applying patch :)

- There's should be no vertical jumping anymore
- There's 1px movement from LEFT to RIGHT (affected by active-link.js, it changed the font weight.)
- There's flashing on Toolbar only. This is loading & rendering the SVG icons.

-------

OFF-TOPIC:
There's some strange behavior in Edge. For example,

In "/drupal8x/admin/reports/fields"

ONLY This line is moving :s
"This list shows all fields currently in use for easy reference."

nod_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -JavaScriptTest

I added only minor changes to the patch, droplet did all the work :) so RTBC

JS review: droplet worked on the patch and I reviewed it and added minor changes a couple of times.
CSS review: by lauriii in #121, #125. Fixed in #126

The way it works is the toolbar is loading in desktop-first mode. Before we had one render method that did several things for many events (in the BodyVisualView.js file). It's now all split up and bound to the relevant events, it avoids re-rendering when not useful.

This fixes the flicker in Edge, and Chrome desktop (there was no flicker in FF). Browsing with a mobile device, there are no changes in the user experience.

yoroy’s picture

Very happy to see we have arrived at a solution for this!

dpacassi’s picture

Applied the patch already to two different sites, very happy with it! Thanks all for the great work!

droplet’s picture

We have 4xx living reviewers & supporters :)
https://www.drupal.org/project/usage/toolbar_anti_flicker

Don't forget this: #2696023: Save Users' Toolbar State config to serverside
Needs some inputs to explain why session saving is bad. To me, it's still scalable. I don't see any bad side effects.

droplet’s picture

Here's some suggestions to deal with flicker issue on contrib theme or moudule after commit:

1. Don't try to understand Toolbar's JS :)
2. Turn of JS
3. Now, you will find a `.toolbar-loading` class.
4. Apply CSS style with `.toolbar-loading` to setup different style (mainly adjust the padding & margin).
Here's two good working example:
Bootstrap Theme (margin-top): #2855957: Remove Bootstrap Flicking
Custom dropdown (expanded the toolbar menu): #2855950: Remove flicker by hide dropdown menu on loading

If you still experience any issue, please tagging `toolbar-flicker` (and drop me a message). I will provide my advice when I can.

Happy coding.

effulgentsia’s picture

I'm very happy to see this RTBC. I haven't started reviewing the patch yet, but just wondering if any automated tests should be added. Any thoughts on that?

nod_’s picture

We only moved code around, didn't delete anything. When we get to removing unused code (like the toolbar-oriented class) that'll be required.

Not sure what kind of tests specific to this issue we could add. Not like the testbot can see the flicker. And adding boilerplate toolbar tests feels like scope creep in this issue.

Cottser’s picture

+++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
@@ -209,12 +234,22 @@
+      // @droplet: Sync Body Classes in real time, not after all scripts execution.

Does this need to be addressed? Should we create a follow-up issue?

droplet’s picture

FileSize
1.55 KB

Just trying to fix the contrib Admin Toolbar Module and I found a problem. I wonder if anyone interested to fix these line in CORE also.

The error is not so visible in CORE but absolutely a potential problem.

In CORE Toolbar, we only adding sub-menus to vertical. At the moment, the whole changes are fast enough to not trigger any annoying element/flashing. But if anything between these JS executions slowing script down, it missing required CSS class & style. The toolbar will render wired. (Admin Toolbar Module is a good example.)

To get rid of this potential bug, it should hide the sub-menus at the earlier point.

This is how the Toolbar classes will be added/removed:
.toolbar-tray-horizontal is adding after .toolbar-horizontal
and
.toolbar-tray-open is alway there when you toggle between horizontal & vertical.

droplet’s picture

#138,

Does this need to be addressed? Should we create a follow-up issue?

A better comment:

Toggle toolbar's parent classes before other toolbar classes to avoid potential flicker and re-rendering.

nod_’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 141: core-toolbar-flicker-2542050-141.patch, failed testing.

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community

Test fail is unrelated

Mixologic’s picture

I added a test to #141 as today we launched the "eslint fails tests" option in drupalci. #141 has one eslint error, so the patch should fail.

Mixologic’s picture

Status: Reviewed & tested by the community » Needs work
droplet’s picture

Status: Needs work » Needs review
FileSize
27.17 KB
3 KB

- Fixed ESLint, Thanks @Mixologic

- Fixed the class adding method to ensure the old class doesn't override. It seems not so testable? Because of we only enable minimal module on testing. Even enabled all modules with current module loading order, the class still correct. :S

- Added [#139]

The last submitted patch, 141: core-toolbar-flicker-2542050-141.patch, failed testing.

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev

@nod_ asked me whether this can land in 8.3. There are actually two questions:

  1. Will we commit it during RC? Gut says "no" here, although I'm not a frontend framework manager.
  2. Will we commit it in an 8.3.x patch release? I am theme- and js-incompetent enough that I cannot properly evaluate the disruption of this. :) The one thing that raised a flag is that this is making changes in Stable to fix the bug (but not Classy)?

I'll ping some theme/frontend maintainerfolk for feedback. However, moving to 8.4.x for now on account of the Stable changes.

Chi’s picture

@xjm, this is the most annoying bug in Drupal 8 for the time being. I would change its priority to "Critical" if it has any formal criteria for this. For many people having this fixed in Drupal 8.3.0 is more important than any other cool features provided by this release.

xjm’s picture

@Chi, I also use Drupal 8 once in awhile, and I don't think I've ever noticed this. :)

That said, if this bug is as irritating as it says in the title, there could be a case for backporting the fix during RC, especially if it is not patch-safe. However, we need a frontend maintainer to weigh in on the BC.

Thanks!

Chi’s picture

FileSize
4.19 MB

I don't think I've ever noticed this.

This might be environment related issue. I've added a short video to demonstrate the problem.

yoroy’s picture

I have never noticed it either untill recently. I forget what was different in my environment. Basically what happens that right after the page seems to have finished loading and you are ready to click on something the whole page moves down a bit because toolbar inserts itself at the top of the page.

It's as if you're choosing your floor in the elevator and right when you want to hit that button they all move a position. And Drupal does this *on every page* load where a toolbar gets shown. It really is extremely annoying and very user unfriendly. From a user experience perspective the case to backport it to 8.3 is strong.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This needs minor reroll after #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase.

I think committing this to 8.3.x is tricky because this contains BC breaking changes. According to our BC policy, markup and CSS in Stable and Classy should be frozen.

I still suggest we commit this also into Stable since this is major enough bug fix that IMO we want to make this available for everyone. Also, the CSS changes included in this patch are low risk (mainly additions and none of the changes change the weight of the selectors).

I also tested this with the Admin Toolbar module which AFAIK is the most popularly used module extending Toolbar.

droplet’s picture

Status: Needs work » Needs review
FileSize
27.17 KB

Status: Needs review » Needs work

The last submitted patch, 154: core-toolbar-flicker-2542050-147.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

"Minor changes, Minor changes. If not, we won't commit it". I always keep it in mind since from Patch in #15, 20 months ago. If not, I may break the JS code seriously. :P

Yeah, that may be a little BC breaking if we considered things seriously. (That broken the loading visual more than final visual I think)

CSS:
As @lauriii said, the CSS changes are lower risk. It's tricky!!!

- It's very easy to fix.
- Basically, the older CSS has higher CSS specificity than new CSS. That meant, YOUR customized CSS style may break the new fixes (flicker). But will not change the final visual

JS:

+++ b/core/modules/toolbar/js/views/BodyVisualView.js
@@ -17,36 +16,25 @@
-      this.listenTo(this.model, 'change:orientation change:offsets change:activeTray change:isOriented change:isFixed change:isViewportOverflowConstrained', this.render);
+      this.listenTo(this.model, 'change:activeTray ', this.render);
+      this.listenTo(this.model, 'change:isFixed change:isViewportOverflowConstrained', this.isToolbarFixed);

There's a lot of JS changes. BUT I think we only need to evaluate the events changes.

droplet’s picture

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

Based on @lauriii's review in #153, I don't think we can backport the current fix to 8.3.x. Also, we will need to create a change record for the BC breaks in Stable especially since our policy does not normally allow them. So, tagging for a change record and marking "Needs work". That will give us a way to fix the bug for all users but also communicate the disruption to theme maintainers.

What we might be able to do after this is committed is create a backport-safe version of the patch that includes only the changes toToolbar and possibly changes to support it in Seven instead of relying on the fix in Stable. That could allow us to fix the annoying bug for many users, without risking disruption of custom themes during a release candidate.

Let's get it fixed for 8.4.x first, and then explore that option maybe?

Thanks!

xjm’s picture

BTW @Chi, the video is a great illustration, thanks. :-) By showing the layout change and jump over and over it makes it clear how vexing it could be.

droplet’s picture

Here's a new version of Toolbar Anti-flicker:
https://www.drupal.org/project/toolbar_anti_flicker

Daniel Schaefer’s picture

Droplet, I've tested your module. I'm sure it's known, but there is still some flickering due to font weight and the shadows seem to be applieded after page load only. A more concerning issue though, when switching between the menu groups (shortcuts, user) there is severe jumping because the shortcuts are loaded first. It's not ideal.

nod_’s picture

What we have a problem with is the vertical shifting of content during page load. It is actively disturbing when administrating a site and is a performance issue. Horizontal flicker while annoying doesn't have those issues.

Anonymous’s picture

+1 for upping the priority to critical. I too hadn't noticed this until recently and ever since it has been driving me nuts. To the point where sometimes I disable the toolbar and just type in URLs when working locally.

droplet’s picture

@Daniel Schaefer,

A more concerning issue though, when switching between the menu groups (shortcuts, user) there is severe jumping because the shortcuts are loaded first. It's not ideal.

Yes, the JS is running after CSS rendering. There're 2 workarounds:

1: Set it to blank until full loading. (If we do, I think we need not change such huge amount of code. Just set the fixed height there.)

2: Kick it out of Drupal.attachBehaviors and change HTML output to the top may reduce the flicker. (didn't test yet and don't think we will approve patch like this.)

Anyway, I think it should sort in the 2nd round for these issues, not now. Thanks.

yoroy’s picture

So, who's up for writing that change record? (see #158) Lets get this over the finish line at least for 8.4 for now :)

Sam152’s picture

Status: Needs work » Needs review

I started one here: https://www.drupal.org/node/2871997

I found it hard to enumerate all of the specific CSS changes being introduced, so I included a diff. If anyone has a better approach, that would be great.

jonathanshaw’s picture

I found it hard to enumerate all of the specific CSS changes being introduced

I tried too and after getting a third of the way through it was already too many to be easily comprehensible.

Anyone messing with the toolbar is going to need to test and rethink their work carefully if they hit a problem, there is not much the change record can do to facilitate things. There are just a lot of CSS tweaks in this patch.

droplet’s picture

Hmmm.. Hope it will help: (If anyone can help to fix my grammar or wordings that will be great ^_^)

In order to improve the front-end performance of the toolbar, some minor CSS changes have been introduced into the toolbar module. To ensure these changes reach the widest group of users possible and due to the low impact nature of the changes, they have also been added to the stable theme.

1.
In this change, we moved 3 default BODY classes rendered by JavaScript from frontend to backend (as default HTML output) in order to remove the flicker problem:

.toolbar-tray-open
.toolbar-horizontal
.toolbar-fixed

** This change make ZERO changes to final visual.

2.
As stated above, we changed some critical CSS rules to rely on default classes from HTML output rather than selectors added by JavaScript. In this case, we use body.toolbar-horizontal rather than nested classes (e.g., .toolbar .toolbar-tray-horizontal)

Here's common pattern, and you can perform "Search & Replace" to fix your customized style:

Search:

.toolbar .toolbar-tray-horizontal

Replace:

.toolbar-horizontal .toolbar-tray

** Note: We only patch necessary CSS rules to lower the impact of this changes. We highly recommended you always use .toolbar-horizontal or .toolbar-vertical on your own themes or modules.

3.
On the other side, we also introduced .toolbar-loading class to indicate the Toolbar loading state and apply some tricks with this class to reduce the flicker issues. The class .toolbar-loading will be removed after Toolbar is fully loaded.

Note: Modules or Themes trying to add Margin or Padding to BODY / HTML tag and Toolbar's parent class (e.g., #toolbar-administration) may break this fixes.

We recommended this simple pattern for style may caused extra flickers:

body:not(.toolbar-loading) .your-custom-style {
	padding-top: 625px;
}
jonathanshaw’s picture

I updated the change record with @droplet's text, and reworked the language slightly. See https://www.drupal.org/node/2871997

This sentence did not make sense to me, I did not understand what it was trying to say:

We recommended this simple pattern for style may caused extra flickers:

body:not(.toolbar-loading) .your-custom-style {
padding-top: 625px;
}

droplet’s picture

Thanks @jonathanshaw!!!

We recommended this simple pattern for style may caused extra flickers:

Actually, my suggestion doesn't work in most cases... We could add the following working example instead:
https://www.drupal.org/node/2855957

jonathanshaw’s picture

OK, I've updated to use the working example you suggested. But please review the whole CR @droplet, I'm just trying to clarify your language, I'm not actually familiar with the issues involved.

Sam152’s picture

I've reread the CR, looking pretty good. The only point of contention for me is:

Modules or themes trying to add margin or padding to the BODY / HTML tag or toolbar's parent class (e.g. #toolbar-administration) may have their appearance disrupted as a consequence of these changes. The solution is to add corresponding offsets to the toolbar...

Is displace.js the standard solution for padding/margins on body? Can we simply indicate anyone using the API won't be affected?

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

Despite #172, I don't think there is actually anything wrong with the CR. It looks like that was also the only outstanding task for this issue.

Lets make this happen for 8.4.x!

droplet’s picture

@Sam152,

Sadly, no simple workaround I can share :( displace.js isn't really used to position things and ALL involved JS are too late. You must use CSS instead.

I won't discuss it's pros or cons right now but usually the sites outside Drupal, it's simply hard-coded the value. For example, WordPress admin bar.

Here's really have another simple workaround but I can't remember why I didn't do it.

We can only make Toolbar sticky on scroll event.

yoroy’s picture

Assigned: Unassigned » lauriii

Assigning to @lauriii to have a look.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 157: core-toolbar-flicker-2542050-157.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
36.13 KB
Sam152’s picture

Status: Needs review » Reviewed & tested by the community
idebr’s picture

Wim Leers’s picture

  • lauriii committed b67adf0 on 8.4.x
    Issue #2542050 by droplet, nod_, dpacassi, Wim Leers, Sam152,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

This does introduce a BC breaking changes according to our BC policy for the Toolbar. These BC breaks are documented in the change record. I couldn't think of a way to fix the bugs described in this issue without making these BC breaks. Making this change gives us a major user facing improvement, and given the nature of the BC break, I'm fine with this change.

I also tested this out of curiosity with few of the most popular modules making changes for the toolbar, and none of them was broken by this change. However, they might still have to apply their own set of changes to make the toolbar not flicker after their changes. There is a stylelint failure regarding a missing space but that can be fixed on commit.

Thank you for everyone for your work on this issue, and sorry for the delay.

Committed b67adf0 and pushed to 8.4.x. Thanks!

dawehner’s picture

This might be my favourite commit of 8.4.x!

yoroy’s picture

Much relieved this is committed now! Thanks @lauriii and everyone who worked on this.

Wim Leers’s picture

I want to celebrate too, but … I don't actually see a difference? :O It's still just as jumpy, and flashing… And yes, I did clear all the caches, including that in my browser.

What is manual testing showing for other people?

Chi’s picture

It did not work for me until clearing browser caches.

plach’s picture

FileSize
327.36 KB

I just installed 8.4.x from scratch and the toolbar menus are always expanded and I cannot collapse them:

Any chance this is related?

droplet’s picture

@plach,

I can confirm the issue (but I'm not sure if it's affected by this patch). Can you open an issue, I will find it out and patching. Thanks.

plach’s picture

  • lauriii committed b5a3346 on 8.4.x
    Issue #2891236 by droplet, krina.addweb: Regression in #2542050: Toolbar...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Chi’s picture

Issue tags: +8.4.0 release notes

This should be the first record under "Important bug fixes" section in release notes.