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:
- 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.
- 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).
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)
| Comment | File | Size | Author |
|---|---|---|---|
| #187 | People___Drupal_8_x.png | 327.36 KB | plach |
| #177 | core-toolbar-flicker-2542050-177.patch | 36.13 KB | nod_ |
| #157 | core-toolbar-flicker-2542050-157.patch | 27.15 KB | droplet |
| #154 | core-toolbar-flicker-2542050-147.patch | 27.17 KB | droplet |
| #151 | toolbar_rendering.mp4 | 4.19 MB | chi |
Comments
Comment #1
wim leersHere'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
Comment #2
wim leersComment #3
dawehnerThank you for creating an issue wim.
Comment #4
Bojhan commentedOMG, yes!
Comment #5
Daniel Schaefer commentedThank 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?
Comment #6
dawehnerYeah every user has to deal with that ...
Comment #7
wim leers+1 to title change. I'm tempted to change it to "EXTREMELY".
Comment #8
Jaesin commentedThe patch in #1 works and is a huge AX enhancement.
Comment #9
dawehnerIts a step forward, IMHO, this is not a bad idea.
Comment #10
dawehner.
Comment #11
wim leers@Jaesin: why is it a huge AX (Authoring Experience?) improvement? In my testing, it barely made a difference :(
Comment #12
droplet commentedwhat kind of flickering? (Screen move down after loaded ? )
Comment #13
effulgentsia commentedI 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.
Comment #14
Jaesin commented@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.
Comment #15
droplet commentedFlicking 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.)
Comment #16
wim leersI don't think this is acceptable? This only works for Bartik.
Comment #17
wim leers#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?
Comment #18
droplet commentedUnfortunately, 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)
Comment #19
Daniel Schaefer commentedI 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
Comment #20
Daniel Schaefer commented@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.
Comment #21
droplet commented@Daniel,
Nice!
have some free time to clean up other issues:
#2559769: Use CSS to position toolbar instead JS
Comment #22
Jaesin commentedI'm not really seeing the flickering in Beta-16 even without this patch.
Comment #23
bkosborneFor what it's worth, this problem is not present for me in FireFox (on a Mac), but very apparent on Chrome and Safari.
Comment #24
wim leers#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.
Comment #25
StuartJNCC commentedAgree 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.
Comment #26
nod_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.
Comment #27
nod_But it needs to be rewritten to be performant. Tried a few things but backbone gets in the way. The really heavy function is
updateTrayOrientationand 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.
Comment #28
wim leersIs #26 ready for final review and manual testing, or do you plan more changes?
Comment #29
pwolanin commentedThe 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.
Comment #30
tarekdj commentedConfirmed!
Also, the vertical toolbar breaks on resizing. See GIF attached.
Comment #31
droplet commentedIt'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.
Comment #32
OnkelTem commentedHi!
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
Comment #33
andrewmacpherson commentedComment #34
sf_wind commentedOh really? We are not going to fix it in 8.0.x? In my opinion, it is not usable...
Comment #35
nod_Still up for 8.0.x, no api change, just optimization.
Comment #36
lokapujyaCan the "back to site", and the displace of the the dropdown menu be rendered without javascript?
Comment #37
joewebmaster commentedI 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?
Comment #38
wim leersAbsolutely.
Currently nobody is working on a fix, so the honest answer is: .
Comment #39
droplet commented@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)
Comment #40
wim leersThat 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.)
Comment #41
Daniel Schaefer commented@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.
Comment #42
droplet commented@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.
Comment #43
Daniel Schaefer commentedYep, 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.
Comment #44
wim leersWe can completely change how the Toolbar is rendered in 8.1 to fix this very nasty UX problem.
Comment #45
joewebmaster commented@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.
Comment #46
droplet commented*** 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:
Pre-render the class
I think this version is possible get into D8.0. :)
Comment #47
droplet commentedWOW. 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: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 :)
Comment #49
joelpittetYeah 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.
Comment #50
droplet commentedEither 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
Comment #51
droplet commentedFix tests
Comment #53
droplet commentedAhh. Actually we didn't need the control item.
Comment #54
droplet commentedComment #56
Daniel Schaefer commentedI'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:
This problem is caused by the display property in line 80 of toolbar.module.css:
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: nonein 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.
Problem here are the
position: absoluteandposition: relativeproperties in line 85 and 80 of toolbar.module.css which prevent theposition: fixedproperty 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: fixedin 203 did the trick.toolbar.module.css (line 203)
After that it seems fine:
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.
Comment #57
Daniel Schaefer commentedI've noticed another thing. The responsive behavior is really strange.
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: absoluteto .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.Comment #58
Daniel Schaefer commentedComment #59
droplet commented@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.
Comment #60
Daniel Schaefer commentedHi 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.
Comment #61
droplet commented** 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!
Comment #62
droplet commentedJust read in other thread, there's only 2 more day before 8.1.0 beta.
Comment #63
Daniel Schaefer commentedJust 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?!
Comment #64
droplet commentedAhh. 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.
Comment #65
geerlingguy commentedAttached 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.
Comment #66
lokapujyaI 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:
But, it can be modified to repaint after the jump.
Comment #67
droplet commented@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.
Comment #68
geerlingguy commented@droplet - After I applied the patch I did a
drush crthen 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.]
Comment #69
iampumaPatch #64 works nicely for me. At least I do not have that flickering annoyance anymore under my skin.
Comment #70
ParisLiakos commentedyes, #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
Unrelated changes
Comment #71
droplet commented@ParisLiakos,
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
We can dropped the CSS changes. But it's not a big deal here. :)
Comment #73
dpacassiHey 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
Comment #75
dpacassiHad an error in my last patch, please test this one.
Comment #76
droplet commented@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.
Comment #77
andrewmacpherson commentedThe patch in #75 has some new functions with odd names, looks like r/l typo.
Re-rolled patch changes these names to:
updateToolbarHeight
setToolbarHeight
Comment #78
dagmarThis should include a docblock like this:
/**
*Implements hook_preprocess_HOOK() for HTML document templates.
*/
Comment #79
dpacassiGood eyes @andrewmacpherson !
I only added the
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.
Comment #80
droplet commented** SKIP THIS COMMENT **
Comment #81
droplet commentedone more change.
Comment #82
jibran@Wim Leers should we add functional-javascript tests for this?
Comment #83
droplet commentedOuch #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.
Comment #84
jonathanshawGiven 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?
Comment #85
droplet commentedI'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.
Comment #86
lokapujyaI think it would be helpful if you could try to describe the architecture change in this patch.
Comment #87
droplet commented@lokapujya,
JS Part:
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)
Comment #88
droplet commented@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.
Comment #89
andypostlooks out of scope but ++
Comment #90
droplet commentedThanks @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 :)
Comment #91
dpacassi@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.
Comment #92
dpacassi@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:

I guess we'll need to investigate this further :-/
Comment #93
droplet commented@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...
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. :)
Comment #95
paulwdru commentedSurprisingly 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
Comment #96
chi commented@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.
Comment #97
paulwdru commented@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
Comment #98
lokapujyaMy 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.
Comment #99
droplet commentedI 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.)
Comment #100
lokapujyaThanks. So, just to help people out, is #91 the patch that should be reviewed right now?
Comment #101
droplet commented@lokapujya,
Yes, #91 is the final patch.
Comment #102
droplet commentedAdd 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.
Comment #103
droplet commentedA new patch is coming soon.
Comment #104
paulwdru commentedGreat 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
Comment #105
Bojhan commented@Wim Could you take a look at this?
Comment #106
wim leers@Bojhan: I'm not a maintainer of the Toolbar module. @nod_ is. Still, reviewed it anyway because you asked.
Needs LTR support
What's the origin of this dimension?
/admin/*paths, right?Woah this is a massive change. Does this mean you can no longer switch between horizontal & vertical orientation?
Nit: s/aviod/avoid/
Nit: two spaces after
activeTab.Nit: s/array()/[]/
This does NOT match what's in
core/modules/toolbar/css/toolbar.module.css.Comment #108
nod_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.
Comment #109
droplet commentedThe 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.
Comment #110
nod_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.
Comment #111
sam152 commentedThis 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 :(
Comment #112
sam152 commentedNW for #106.
Comment #113
droplet commented- 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.
Comment #114
nod_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.
Comment #115
nod_Fixed mobile load. Still no flicker.
Comment #117
nod_green
Comment #118
droplet commentedAh 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)
Comment #119
nod_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
Comment #120
droplet commented@nod_, makes sense. Any reviewers?
Comment #121
lauriiiI 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.
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.
Do these selectors actually need the .toolbar class?
Comment #122
droplet commentedWill it work:
// .toolbar-loading is required by Toolbar JavaScript to pre-render markup style to avoid extra reflow & flicker.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
!importantthere.)Many Toolbar's CSS coded like this:
.toolbar .toolbar-bar .toolbar-tabTo me, I will do this:
.toolbar .toolbar-tabEven this instead:
Comment #123
nod_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.
Comment #124
droplet commentedThis is used by my module to fix the vertical movement and introduced from #113 accidently. We can remove it from CORE patch.
Generally speaking,
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-openshould 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. :)
Comment #125
lauriiiFair point. Let's keep the selectors as is now and do the clean-up in another issue.
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
Let's do so.
Comment #126
nod_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 :)
Comment #127
tameeshb commentedFew more comment style fixes from toolbar.module.css.
Please review
Comment #128
droplet commented#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.
Comment #129
jonathanshawReuploaded 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..
Comment #130
droplet commented@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."
Comment #131
nod_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.
Comment #132
yoroy commentedVery happy to see we have arrived at a solution for this!
Comment #133
dpacassiApplied the patch already to two different sites, very happy with it! Thanks all for the great work!
Comment #134
droplet commentedWe 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.
Comment #135
droplet commentedHere'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.
Comment #136
effulgentsia commentedI'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?
Comment #137
nod_We only moved code around, didn't delete anything. When we get to removing unused code (like the
toolbar-orientedclass) 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.
Comment #138
star-szrDoes this need to be addressed? Should we create a follow-up issue?
Comment #139
droplet commentedJust 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.
Comment #140
droplet commented#138,
A better comment:
Toggle toolbar's parent classes before other toolbar classes to avoid potential flicker and re-rendering.
Comment #141
nod_Updated comment
Comment #143
jonathanshawTest fail is unrelated
Comment #144
MixologicI 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.
Comment #145
MixologicComment #146
droplet commented- 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]
Comment #148
xjm@nod_ asked me whether this can land in 8.3. There are actually two questions:
I'll ping some theme/frontend maintainerfolk for feedback. However, moving to 8.4.x for now on account of the Stable changes.
Comment #149
chi commented@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.
Comment #150
xjm@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!
Comment #151
chi commentedThis might be environment related issue. I've added a short video to demonstrate the problem.
Comment #152
yoroy commentedI 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.
Comment #153
lauriiiThis 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.
Comment #154
droplet commentedComment #156
droplet commented"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:
There's a lot of JS changes. BUT I think we only need to evaluate the events changes.
Comment #157
droplet commentedComment #158
xjmBased 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!
Comment #159
xjmBTW @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.
Comment #160
droplet commentedHere's a new version of Toolbar Anti-flicker:
https://www.drupal.org/project/toolbar_anti_flicker
Comment #161
Daniel Schaefer commentedDroplet, 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.
Comment #162
nod_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.
Comment #163
Anonymous (not verified) commented+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.
Comment #164
droplet commented@Daniel Schaefer,
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.
Comment #165
yoroy commentedSo, who's up for writing that change record? (see #158) Lets get this over the finish line at least for 8.4 for now :)
Comment #166
sam152 commentedI 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.
Comment #167
jonathanshawI 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.
Comment #168
droplet commentedHmmm.. 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:
** 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-horizontalrather 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:
Replace:
** Note: We only patch necessary CSS rules to lower the impact of this changes. We highly recommended you always use
.toolbar-horizontalor.toolbar-verticalon your own themes or modules.3.
On the other side, we also introduced
.toolbar-loadingclass 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:
Comment #169
jonathanshawI 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:
Comment #170
droplet commentedThanks @jonathanshaw!!!
Actually, my suggestion doesn't work in most cases... We could add the following working example instead:
https://www.drupal.org/node/2855957
Comment #171
jonathanshawOK, 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.
Comment #172
sam152 commentedI've reread the CR, looking pretty good. The only point of contention for me is:
Is displace.js the standard solution for padding/margins on body? Can we simply indicate anyone using the API won't be affected?
Comment #173
sam152 commentedDespite #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!
Comment #174
droplet commented@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.
Comment #175
yoroy commentedAssigning to @lauriii to have a look.
Comment #177
nod_Reroll for #2818825: Rename all JS files to *.es6.js and compile them
Still working.
Comment #178
sam152 commentedComment #179
idebr commentedClosed #2882654: Header fluctuates whenever user redirects to any of the menu item or anywhere as a duplicate
Comment #180
wim leersFor #179.
Comment #182
lauriiiThis 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!
Comment #183
dawehnerThis might be my favourite commit of 8.4.x!
Comment #184
yoroy commentedMuch relieved this is committed now! Thanks @lauriii and everyone who worked on this.
Comment #185
wim leersI 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?
Comment #186
chi commentedIt did not work for me until clearing browser caches.
Comment #187
plachI just installed 8.4.x from scratch and the toolbar menus are always expanded and I cannot collapse them:
Any chance this is related?
Comment #188
droplet commented@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.
Comment #189
plachSure: https://www.drupal.org/node/2891236
Comment #192
chi commentedThis should be the first record under "Important bug fixes" section in release notes.
Comment #193
idebr commentedRe Wim Leers #185:
I still noticed a flicker on my local environment after updating to 8.4.x. It turns out this was caused by the Toolbar menu items added by the Devel module, see #2881521: Toolbar Handler should use toolbar.menu_tree instead of menu.link_tree
The fix for the Devel module is included in latest stable release, so updating it may help.
Comment #194
sillo commentedI really dont understand why this is marked as fixed? Its definately not fixed.
Everything about the D8 backend is a mess (sorry i dont mean to offend anyone).
Admin menu is SLOW and still renders way after the page is already loaded
Menu link weight shows the input field for weight before it finally hides it through JS and shows the handle instead.
This is just the tip of the iceberg. The general JS rendering of the backend should really be looked at, because its not optimal at all.
Please re-open this issue and let's fix it.
Comment #195
chi commented@sillo, can you file a separate issue on this?
Comment #196
ndf commented#193, #194, #195 here the follow-up issue: #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays
Comment #197
effulgentsia commentedFor anyone who remembers if what was committed here ever solved the re-render for you, if you're able to, please comment on #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays if you're currently seeing the flicker that's described there. Is that issue identifying a regression that's occurred after this issue landed, or is it finding something incomplete about what was committed here? Thanks in advance for any insight you're able to offer.
Comment #198
chi commented@effulgentsia, I do remember that worked well for some time after this fix landed. At least without Big Pipe. Also the flickering we were fixing here (both horizontal and vertical) was different from the one described in #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays. You can check the recording for comparison.
Another important point is that the new horizontal flickering only happens (for me) when Big pipe module is enabled. We might not notice this here because Big Pipe was not widely used at that time (see #2840392: Enable BigPipe by default in the Standard install profile).
Comment #199
paulwdru commented@sillo
1000% Agree with you, Drupal's backend is still a complete mess despite being added with many new major features, I still have to give up D8 in 2020 as my partners can't really stand with the Super Duper Annoying Flickering which renders D8 as a half-baked & immature CMS at first glance