Problem/Motivation

Toolbar using client-side session (localStorage) to store the toolbar state now. And this is default to render in horizontal mode. When users toggle to Vertical Mode, on each page load there's a jumping after page is fully loaded. Due to browsers limitations, frontend performance concern and Drupal's design (eg. Can't using inline scripting / Can't delay rendering after JS), we must pre-render CSS class for toolbar in backend.

About `horizontal mode` we fixing in #2542050: Toolbar implementation creates super annoying re-rendering.. (@see Comment #61, Code explained itself. )
More ref about browser side: http://www.html5rocks.com/en/tutorials/internals/howbrowserswork/

** @droplet: noted that, there's nothing about Backbone performance or bad coding on JS side.

Proposed resolution

Possible Workarounds (by @droplet):
A: Using Backend session.
B: Delay rendering after Page is fully loaded. (BAD IDEA)
C: Adding Anime to make it looks more smooth. (Needs proof)
D: Drop `horizontal mode` for desktop users.

Remaining tasks

- TBA

User interface changes

- TBA

API changes

- TBA

Data model changes

- TBA

Comments

droplet created an issue. See original summary.

DuaelFr’s picture

Isn't it possible to just use a session setting? It seems overkill to store this little setting in the database.

droplet’s picture

I think you refer to server side session settings right? It's also involved with DB so that works. Just we may have ONE jump at first time to visit the site

DuaelFr’s picture

That'd also be easier to store it in a cookie even if the EU regulations are nuts about that.
Cookies can be written in JS then read in PHP unlike session variables that can only be written and read by PHP.

ParisLiakos’s picture

Definitely session. Having it in session means it can be toggled without having to visit the settings form.
Even if its on horizontal mode every time someone logs in as default

DuaelFr’s picture

Having the setting in a session variable requires that clicking on the switcher triggers an AjaX call to a new endpoint that is going to set the value.
Having the setting in a cookie requires nothing else than writing the cookie with client side JS on click on the switcher.

Both requires about the same amount of code to read the value from its storage and output the right CSS class.
As a bonus, the cookie can be set as a cache context but the session variable cannot (afaik).

ParisLiakos’s picture

Having the setting in a session variable requires that clicking on the switcher triggers an AjaX call to a new endpoint that is going to set the value.
Having the setting in a cookie requires nothing else than writing the cookie with client side JS on click on the switcher.

True! Also implementation is easier.
So:

  1. Javascript sets the cookie
  2. PHP reads the cookie and sets a body class?
  3. CSS use this body class as a selector to apply the correct default toolbar styles

I think something like the above should be enough, and easy (why not merge it to #2542050: Toolbar implementation creates super annoying re-rendering. )

DuaelFr’s picture

Do that class really have to be on the body?
I may be an issue for caching and performances.

droplet’s picture

@DuaelFr,

Umm.. For Vertical Mode, YES. Adding padding to left.

DuaelFr’s picture

I asked on IRC for some cache experts to have a look at this issue.
Let's wait a bit for their opinion and refine the IS and the title now we have some more clear ideas. :)

Wim Leers’s picture

+100 to Needs issue summary update

It's not at all clear why we want to do this in the first place. Please explain it in the IS.

On first glance, it's a very very bad idea for performance & scalability to store this on the server side (for every session or user). Even more so because it's already being saved on the client side. This can only be answered with more clarity if there is an actual issue summary.

droplet’s picture

Title: Save Toolbar state into DB » Save Users' Toolbar State config to serverside
Issue summary: View changes
droplet’s picture

Issue summary: View changes
droplet’s picture

Attached a patch with state storage.

I would like to hear what you're concerned on the performance side.

eg. extra cookie ?
eg. no more full page caching in frontend for non-admin user ?
eg. can't proxying ?

** We should think in average site builders side. **

kostyashupenko’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: cookie-diff.patch, failed testing.

Bcwald’s picture

Another reason this is important is because Safari private browsing mode on both iOS and OSx dont support SetItem, which causes ALL of the javascript on your site to break. AFAICT this is the only place in core that sets a variable with LS.

See my issue here: https://www.drupal.org/node/2711707

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.