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

CommentFileSizeAuthor
#14 cookie-diff.patch3.65 KBdroplet
#14 full.patch15.51 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

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

droplet’s picture

droplet CreditAttribution: droplet commented

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

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

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

ParisLiakos CreditAttribution: ParisLiakos commented

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

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

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

ParisLiakos CreditAttribution: ParisLiakos commented

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

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

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

droplet’s picture

droplet CreditAttribution: droplet commented

@DuaelFr,

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

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Issue tags: +Needs issue summary update, +Needs title update

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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

+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

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

droplet CreditAttribution: droplet commented
Issue summary: View changes
droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
full.patch15.51 KB
cookie-diff.patch3.65 KB

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

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Active » Needs review

Status: Needs review » Needs work

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

Bcwald’s picture

Bcwald CreditAttribution: Bcwald commented

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.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Related issues: +#2711707: Use of LocalStorage setItem not supported in private browsing in safari, breaking all JS on the site.

Adding #2711707: Use of LocalStorage setItem not supported in private browsing in safari, breaking all JS on the site. as a related issue per #17. In #2711707-7: Use of LocalStorage setItem not supported in private browsing in safari, breaking all JS on the site., I asked whether it makes sense to close that as a duplicate of this one.

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.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Smile commented
Issue tags: -JavaScript +JavaScript
Related issues: +#1863824: Use the temp store to remember the last state of the toolbar per user and reload it

another approach without a cookie

droplet’s picture

droplet CreditAttribution: droplet commented

wow. I don't know the toolbar exists that long.

Note: EU law is about tracking, not has a cookie or not.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.