Reduce toolbar user button related browser reflow

Transferring the tag that was removed when this was rescoped to its own issue. 😅 We really do need to be careful here, so explicit security-minded review is required before this should land.

Thanks!
-Derek

This issue now partly reverts 📌 Deprecate/remove js-cookie dependency Fixed , adding that as definitely related.

Added some review comments on the MR. 1 is just a clarifying comment, 1 a nit, 1 something of substance I think we should fix before this proceeds. Hence NW.

I'm no longer an active member of the sec team, so I don't feel authorized to remove the security review tag I just added, but my quick read of the MR and I don't see anything to complain about from that perspective. You're only setting the cookie for authenticated users. You change it if the username changes. You clear it if the user logs out. I assume the place we're printing this username is already escaping the output to prevent XSS, etc, so I don't see how it matters if we get the untrusted string from a cookie or from a lazy builder. Not a thorough review, but an initial "all seems reasonable to me". 😅

Gotta run for client work, but excited to see progress on all these toolbar UI issues!

Thanks,
-Derek

In Europe we have Data Protection Rights (GDPR) and in germany we have a law calles TTDSG for cookies. by this law the user has explicetly to opt in BEFORE a cookie is set - except it is absolutely needed to deliver service (like Session-Cookie). With this idea for additional cookies any Drupal Site in germany needs an additional opt-in for this cookie, because "reducing repaints" is not "absolutely needed to deliver service". Maybe you can do this optional in a theme. But please no unnecessary cookies in core.

From my view not a good idea.

@dww brought this issue up in Slack and I also mentioned the GDPR implications in Europe of storing personal data (the username) in a cookie, before seeing @jan kellermann's comment. I am not a lawyer but I agree that this will likely be problematic; as I understand it European sites that use the toolbar module would have to allow users to explicitly opt in to this feature to stay within the current legal guidelines.

Does it have to be in a cookie? user_js_settings_alter() already puts the user ID in drupalSettings.user.uid so it would be trivial to add the username here too.

In Europe we have Data Protection Rights (GDPR) and in germany we have a law called TTDSG for cookies. By this law the user has explicitly to opt in BEFORE a cookie is set - except it is absolutely needed to deliver service (like session-cookie). With this idea for additional cookies any Drupal site in germany needs an additional opt-in for this cookie, because "reducing repaints" is not "absolutely needed to deliver service". Maybe you can do this optional in a theme. But please no unnecessary cookies in core.

From my view not a good idea.

The cookie approach has been the active approach for a while in 🐛 Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays Fixed , which addresses many toolbar issues beyond the admittedly more trivial one here. However... I recently had to change that approach to ensure there weren't caching conflicts. With the new approach, it may be possible to simply use localStorage and not have to use Cookies at all.. Since it sounds like the cookie would add major annoyance for German users (and perhaps ones other countries too) it seems like finding a way to accomplish this with localStorage is necessary.

I'm going to postpone this until I can confirm a localStorage approach works in 🐛 Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays Fixed

@bnjmnm

> it may be possible to simply use localStorage and not have to use Cookies at all

Any change in the browser needs explicetly to opt in before the change is made. Changes in the browser means cookies, localStorage and every data saved in the browser.

See german law here: https://dsgvo-gesetz.de/ttdsg/25-ttdsg/

bnjmnm → closed merge request !3762

The username does not need to be stored to prevent the reflow, it's only necessary to know how much space to set aside for the name that will eventually be added via lazy loader. The width change is significantly more jarring than the name popping in. The user-button-width MR uses this approach.

Removing the security review tag, as the new approach doesn't involve storing the username on the client side at all, so this alleviates any GDPR or other security concerns.

Changes look good.

Did some basic checking of the toolbar
Refresh the page a number of times
Go to several different pages
Cleared cache a few times

Didn't notice any issue.

Committed 8149f97 and pushed to 10.1.x. Thanks!

Yay, thanks everyone!

Always love it when everyone who participated in an issue did so meaningfully and all users are credited. 🎉 Refreshing! 😅

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