Page MenuHomePhabricator

[Spike 8hrs] Decide how to persist state of collapsible sidebar across sessions for logged-in users, logged-out users
Closed, ResolvedPublicSpike

Description

User Story

As an editor, I would like to have a persistent state for my sidebar, so that I don't have to change the state every time I start a new session

Implementation

T255727: Make collapsible sidebar persistent for logged-in users
T257169: Proposal: Make collapsible sidebar persistent for logged-out users

Task implemented

For logged-out/unregistered AND logged-in users (by @Demian):
DEMO with all header improvements, including drop-down usermenu (patch 589808 + patch 606436)
DEMO with limited content width (patch 607188)

Acceptance criteria

  • Make collapsible sidebar persistent across multiple sessions for logged-in users (while they're still logged-in)
  • Make collapsible sidebar persistent during a single session for logged-out users (while they switch pages)
  • (nice to have) make sidebar globally persistent for logged-in users (across devices, across logging-in/logging-out)
  • the user preference is marked as hidden so it doesn't show up on Special:UserPreferences

Note on default states

INITIAL STATE: sidebar is default on for all users
EVENTUAL STATE: sidebar is default off for anons, default on for all other users

Developer notes

For persistent state we have several options - localStorage, cookies and user preferences. Given we do not want reflows for users user preferences seems like the correct and only choice here. (EDIT: reflows do not affect proper implementations)

Risk 1 - user preference may not save.

owever there is a change on slow connections that an update to state is not completed before the user navigates to a different page. In that situation e.g. hamburger is clicked and then menu item is quickly clicked the menu may appear collapsed.

Risk 2 - multiple saves? (only server-side solution)

We'll likely want to update the hidden user preference on every click to the hamburger click. If some users keep clicking it in the same session this will result in multiple HTTP requests. We likely want to debounce the saving for at least a second, but that will increase the risks of 1.

Risk 3- future anonymous support?

Note if we ever need to make this work for anonymous user's we'll likely to have to rethink this whole idea. It won't work in that situation.
EDIT: The client-side implementation for logged-out users can be integrated with the server-side implementation for logged-in users.

Alternative?

Some editors might prefer an explicit "Always show menu expanded" as this avoids all the risk above.

Questions

  • What does "globally persistent for logged-in users" mean? Across all wikis (e.g., GlobalPreferences), simply persistent across all sessions, or something else?
  • Do we need per device awareness? E.g., on a tablet, maybe an editor's preference is to hide the menu by default but on a large monitor to show it by default. On the Android native app, we built a similar feature that would sync user options across devices (like GlobalPreferences) that some considered an anti-feature since they wanted different preferences on different devices.
  • What are the tradeoffs? The risks (cons) are identified above but what are the pros of each? Some further discussion may be useful if it hasn't been had already. Slack notes.
  • (Covered by risk 3 but framed a little differently) If latest mode is enabled on a wiki by default for anonymous users, is it considered a nice-to-have feature to support this collapse state option for anons or would it be unused?
  • How is this setting changed? The AC says "the user preference is marked as hidden so it doesn't show up on Special:UserPreferences" so is this toggled by interactions with the sidebar? Is that a good idea or should we consider a default preference state and a session (across pages) state distinctly?

Spike outcome

Jan shared the following presentation:


After considering all the implications we decided to limit this to T255727 for now.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I think it's important to consider the anonymous use-case at this stage, since it makes a lot of sense from an end-user point of view.

Any javascript that executes before page-load and reads from localStorage is going to have a negative performance impact. We should consider that a last resort (and run it by the performance team).

Could we set a cookie for anonymouse users? I image that doing so would double our cache size, since we'd be storing two cached versions of the page, one with and one without sidebar. I know next to nothing about our caching infrastructure, so I'm curious if this is possible.

Could we set a cookie for anonymouse users? I image that doing so would double our cache size, since we'd be storing two cached versions of the page, one with and one without sidebar.

+1, maybe @phuedx knows? If we think doubling the cached pages is doable, the test wiki group would be a great evaluation.

I think this topic came up in the context of backend isomorphic rendering but I don't remember the answer: is there any flexibility to cache all page HTML except the sidebar button? It would be cool if we could inject the button with an open or closed state based on the web request and leave all the other caching as is. It seems like the simplest possible use case of separating content and chrome. I guess I'm wondering what flexibility we have to change this functionality and what the long term effects of that change would be.

Also, we know it's possible to get different HTML for anons based on query params like:

https://en.wikipedia.org/wiki/Banana?uselang=he => `<html class="client-nojs" lang="he" dir="rtl">...`
https://en.wikipedia.org/wiki/Banana?uselang=en => `<html class="client-nojs" lang="en" dir="ltr">...`

If I recall correctly, the mobile site's beta mode worked for anons via cookies. I think it added a beta class to the HTML for beta users only.

I guess the difference in Vector is that this deviation only requires one click whereas on mobile it required two clicks.

Stephen's right. Beta worked via cookies yes. It fragmented the cache by splitting the varnish cache between a beta mode and stable mode. It however has special handling. You may recall an issue with an A/B test recently that almost brought down all the wikis as it added a cookie and every cookie has the impact of turning off caching (T235949#5592886).

I think special cookie handling would be a wasteful way of splitting the cache, as the only difference between the two modes would be a class on the sidebar. I'd much rather split the cache in Varnish to give the legacy mode to anonymous users :) The downside of splitting the cache, is any split will cause the mode that's used the less will have more cache misses.

We may be able to send a header with requests to tell them to skip the Varnish cache in a similar way to X-Debug but that obviously means slower page requests and I'm not sure if ops would be happy about that as it could be a potential vector for attack (imagine if some JS was added to MediaWiki:Common.js that suddenly expanded everybody's sidebar by default - 100% of traffic would be skipping the cache!).

I think the only practical solution I can see right now are:

  1. a script tag at the top of the page that appends a class to the sidebar based on a client side cookie or localStorage. That script would need feature detection, would need to live in the HTML and be hard to maintain as it would run on every single browser (including grade C).
  2. Change our caching infrastructure to allow edge side includes (no existing support so large project)
  3. Accept delay and show annoying flash

Proof-of-concept LocalStorage solution, <350 bytes below-fold: patch 592328
Logged-in + logged-out. No server side setting -> not transferred to different devices.

I think this topic came up in the context of backend isomorphic rendering but I don't remember the answer: is there any flexibility to cache all page HTML except the sidebar button? It would be cool if we could inject the button with an open or closed state based on the web request and leave all the other caching as is. It seems like the simplest possible use case of separating content and chrome. I guess I'm wondering what flexibility we have to change this functionality and what the long term effects of that change would be.

tl;dr: Not yet.

There've been a number of efforts around cleanly separating content and chrome so that the latter can be varied efficiently and independently from the latter, with the earliest dating back to 2005. The Partial Page Caching RfC (see https://www.mediawiki.org/wiki/Requests_for_comment/Partial_page_caching and T34618) was closed in favour of T111588: RFC: API-driven web front-end and T106099: RFC: Page composition using service workers and server-side JS fall-back.

Demonstration of the above client-side solution (patch 592328): PatchDemo (DELETED)

Update: DEMO (patch 607188)

Jdlrobson renamed this task from Make collapsible sidebar persistent across sessions for logged-in users to [Spike] Make collapsible sidebar persistent across sessions for logged-in users.May 18 2020, 4:14 PM
Jdlrobson added a project: Spike.
Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptMay 18 2020, 4:14 PM

Today I remembered in SHDT that the option to save the sidebar state in the user_properties table for logged-in users might raise very similar concerns to when we worked on the AMC Outreach Drawer (T226068).

During that work, we were initially planning to save the drawer state (whether a logged-in users had seen the drawer) in the user_properties table as well, but ended up using localStorage instead due to concerns of the frequency/volume of writes to the database as well as the temporal nature of the records inserted given the expected finite period the campaign was expected to run. A review of this discussion can be seen at T231044.

Therefore, it might be worthwhile to run this task by someone from the SRE team before/if we go down the user_properties table route.

ovasileva renamed this task from [Spike] Make collapsible sidebar persistent across sessions for logged-in users to [Spike] Make collapsible sidebar persistent across sessions for logged-in users, for sessions for logged-out users.May 20 2020, 4:19 PM
ovasileva renamed this task from [Spike] Make collapsible sidebar persistent across sessions for logged-in users, for sessions for logged-out users to [Spike 8hrs] Make collapsible sidebar persistent across sessions for logged-in users, for sessions for logged-out users.
ovasileva assigned this task to Jdrewniak.

Today I remembered in SHDT that the option to save the sidebar state in the user_properties table for logged-in users might raise very similar concerns to when we worked on the AMC Outreach Drawer (T226068).

During that work, we were initially planning to save the drawer state (whether a logged-in users had seen the drawer) in the user_properties table as well, but ended up using localStorage instead due to concerns of the frequency/volume of writes to the database as well as the temporal nature of the records inserted given the expected finite period the campaign was expected to run. A review of this discussion can be seen at T231044.

  1. I recall a request that this setting be saved per device, which is quite reasonable: I wouldn't want the same setting on a medium-sized iPad and a wide screen. How will that be addressed?

As I see the client-side localStorage solution is a fixed necessity for unregistered/logged-out readers, whom make up the majority of users and also works for logged-in users to for many use-cases, but not all. The server-side setting would be an extra only for editors. I wonder whether the benefits of that solution are worth the price, so I'll try to gather what benefits I envision. I'll probably miss something, feel free to extend this list.

What use-cases localStorage solves:

  • remembering the sidebar state in one browser session: while navigating wiki links, or opening them in new tab
  • remembering the state between browser sessions if localStorage (cookies, practically) are enabled

What a server-side setting adds (for logged-in users only):

  1. saving the state in incognito mode / localStorage disabled
  2. saving the state between browsers (if someone uses more browsers)
  3. saving the state between workstations
  4. saving the state between devices (unwanted feature)

IMHO 1) and 3) in the last list are beneficial, 4) is trouble if you decide to save per device. I think it's worth considering whether these use-cases warrant creating a significantly more complex solution in addition to using localStorage, which I assume can't be avoided.

Discussed this with @alexhollender - here's so thoughts so far:
Logged-out users.
Main use case: As an anonymous reader or editor of Wikimedia projects, I would like the ability to access pages linked to in the sidebar with ease so that I can continue my required workflow
Removing the persistence requirement for logged-out users does not heavily interfere with the use case above. Also, since the sidebar will be default-off, it also doesn't create an odd experience when closing. I think we can remove this requirement
Logged-in users
Main use case: As a logged-in users, I would like the ability to collapse the sidebar across multiple pages, so that I can focus on reading the content of the pages themselves
The main worry here is around the lack of ability to predict whether users would collapse the sidebar consistently when needed. It is possible that we would be presenting the sidebar on every page for logged-in users without the users actually wanting it there. Measuring this would be difficult as it cannot be proxied by overall sidebar usage. Thus, I think we should continue exploring this requirement.

Next steps:

  • It would be nice to play with the collapsible sidebar + lack of persistance + fixed width for logged-out users. @Jdrewniak mentioned this would be possible to add to the prototype.
  • (not high priority) I'm curious about the FOUC if we were to have the sidebar persist for anons. If the content is already fixed, perhaps this flash wouldn't feel as shocking as we expect. Is there a way to test this?
  • While I think we'll go forward with the sidebar persistence for logged-in users, perhaps we can potentially remove it as a blocker to deployment. @alexhollender - thoughts on this? I think the next step would be to create that task and get an estimate of risk and maybe time.

Next steps:

  • It would be nice to play with the collapsible sidebar + lack of persistance + fixed width for logged-out users. @Jdrewniak mentioned this would be possible to add to the prototype.

I've made a prototype DEMO saving the sidebar state for logged-out AND logged-in (only in one browser / device) to show this solution is possible in MediaWiki without FOUC.

  • (not high priority) I'm curious about the FOUC if we were to have the sidebar persist for anons. If the content is already fixed, perhaps this flash wouldn't feel as shocking as we expect. Is there a way to test this?

There seems to be a common misunderstanding regarding client-side preferences. In fact, localStorage is a synchronous API, thus using it in the above-the-fold HTML or a synchronously loaded JS file, there is no FOUC. The misunderstanding comes from the adapted way we use ResourceLoader: loading JS asynchronously. Simply, JS is not used in MediaWiki the way using localStorage requires. This is, however, only a matter of what we are used to, not a technical barrier. The performance price of synchronous JS is a few hundred bytes on the wire (uncompressed), which is negligible.

  • While I think we'll go forward with the sidebar persistence for logged-in users, perhaps we can potentially remove it as a blocker to deployment. @alexhollender - thoughts on this? I think the next step would be to create that task and get an estimate of risk and maybe time.

If synchronizing sidebar state between browsers and devices is not part of the use-cases, then the solution for logged-out users works 100% for logged-in users without any server-side database storage need. With that compromise, a few hundred hours of developer time can be saved. Worth considering.

The performance price of synchronous JS is a few hundred bytes on the wire (uncompressed), which is negligible.

@Demian, although the bytes shipped might be small, I think the performance impact of this approach could be more substantial than that.

The approach in your demo, which does eliminate the FOUC, requires synchronously executing a script tag in the <head> or <body> tag (which blocks rendering) that
a. Reads from localStorage (synchronously)
b. modifies the DOM (changing the state of the checkbox from checked to unchecked)
and does so for all anonymous users and all browsers (including grade C)

For these reasons I’m very doubtful that our Performance-Team team would approve of this approach.

The approach in your demo, which does eliminate the FOUC, requires synchronously executing a script tag in the <head> or <body> tag (which blocks rendering) that

The sidebar is below-the-fold: navigation after content (T240489). The unavoidable result of this is that the initial render (above-the-fold) is without the sidebar. This can be seen as a flicker of the sidebar, similar to what's seen on youtube.
The <script> tag I've added is right above the sidebar, below the content, thus only delaying the render of the sidebar, if at all. Initial rendering of the content is not affected.

Accordingly, the rest of the concerns are not issues. Performance concerns are relevant for the above-the-fold content, which is unaffected by this solution.

Sidenote: this is not Flash-Of-Unstyled-Content (FUOC), the sidebar's content is not there yet. By the time it's first rendered it's fully styled.
This would be Flash-Of-Invisible-Content. Also not Flash-Of-Invisible-Text (FOIT), that can have a visible skeleton, thus it is different imo.

I see, however, that there are assumptions regarding the performance of localStorage - or the lack thereof -, therefore I'll detail the actual performance impact of this solution.
It'll be a long read.

The performance price of synchronous JS is a few hundred bytes on the wire (uncompressed), which is negligible.

@Demian, although the bytes shipped might be small, I think the performance impact of this approach could be more substantial than that.
The approach in your demo, which does eliminate the FOUC, requires synchronously executing a script tag in the <head> or <body> tag (which blocks rendering) that

Yes, synchronous execution is the point to finalize the DOM before rendering. It does delay rendering, but only of the sidebar. Everything has a price, let's see in detail how much is that, not just assume something.

The initialization of the sidebar state can be done either server-side in php+sql or client-side in JS+localStorage: the delay is either here or there. The speed difference between LocalStorage and SQL is negligible for one item, for many requests LocalStorage would be faster: the storage is in the same process, while the relational DB is on a separate box.

a. Reads from localStorage (synchronously)
b. modifies the DOM (changing the state of the checkbox from checked to unchecked)

One read from localStorage + one attribute change to the sidebar DOM, before it's laid out and rendered.

A DOM change is already done in the first inline script on all pages, does not seem to be a concern:
document.documentElement.className="client-js";

Reading from localStorage is fast (see measurements on stackoverflow), and we are talking about one single item only, but I have no information about the one-time initialization cost of localStorage. Though I don't expect that to be above 100ms, it's worth to do some measurements. It won't be easy to measure a one time per pageload event reliably, but I'll make a test when I have some time.

and does so for all anonymous users and all browsers (including grade C)

Also, that's the point. This is a solution for both logged-in and anonymous, expect for syncing the sidebar state between browsers and devices for logged-in users, which I think is not worth the effort and some cases is an unwanted feature, such as desktop -> mobile and vice versa.

For these reasons I’m very doubtful that our Performance-Team team would approve of this approach.

Decisions should be based on real, measured performance impact. Everything has a price, but that price can be pennies. In the above reasons I don't see anything more than that.
I see, however, that there are assumptions about the performance impact of client-side JS and localStorage, but I haven't seen any technical reasons for those assumptions within the DI project. I wonder where these come from.
As I haven't seen discussions from the perf team regarding this topic, I'd be interested in reading those, if someone gives pointers to it.

Change 592328 had a related patch set uploaded (by Aron Manning; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [DNM-POC] Demo: Sidebar checkbox in header, Sidebar separately in #navigation

https://gerrit.wikimedia.org/r/592328

@Demian it's hard to compare the performance between a front-end and server-side solution, since for logged-in users the user_properties table is accessed on each request and the results are cached afterwards.

Aside from performance though (which as you mention, can be objectively measured to determine the impact) the inline script is problematic when it comes to our caching infrastructure. Our HTML is cached for up to two weeks, whereas the JS/CSS served by ResourceLoader is updated within 5 minutes. We've had numerous issues in the past with CSS served along with cached HTML, and because of this we typically leave old CSS around for 30 days before we delete it. Placing this script inline with the HTML makes it susceptible to similar caching issues, which could cause maintenance problems down the line.

So even though I agree that the inline-script solution does provide a better user experience (making it available for anonymous users, and being device specific), for the reasons mentioned above we're currently favouring limiting this feature to logged-in users only, and storing it as a user-preference.

@Demian it's hard to compare the performance between a front-end and server-side solution, since for logged-in users the user_properties table is accessed on each request and the results are cached afterwards.

Thanks for the pointer. That might even be faster than localStorage, with a few milliseconds. As you haven't responded to the fact that this delay - however long - on the client side happens below-the-fold, therefore it is irrelevant, can I assume that you agree with that? This also means that the prevalent assumption about localStorage causing delays does not apply to this task. As I see that false assumption predetermined that the modern and simple solution was not even investigated. Unfortunately, the chosen solution builds on a well established, decade old design practice. I understand that's the comfortable path to take, but I would argue that this introduces quite some technical debt and complexity - most notably the issue with desktop->mobile transfer of the preference. However, I'm not advocating against a server-side solution, but for a client-side solution. The two can coexist. I find the latter more important as it affect the millions of readers, not just the thousands of editors.

Aside from performance though (which as you mention, can be objectively measured to determine the impact) the inline script is problematic when it comes to our caching infrastructure.

The shortcomings of the caching design (not even the infrastructure) is worth another essay. However, I'm only interested in the actual consequences in the solution I've presented. Exact case-studies of what state these caching inconsistencies can result in would help us understand the actual impact, without making assumptions. I'll present one below.

Our HTML is cached for up to two weeks

It has been reduced to 7 days in the last change.
EDIT: it's typically 4-7 days, 2 weeks guaranteed.

whereas the JS/CSS served by ResourceLoader is updated within 5 minutes.

JdlRobson said 10 and 30 minutes (both). I'd prefer to know the real value. Where is this configured?

We've had numerous issues in the past with CSS served along with cached HTML, and because of this we typically leave old CSS around for 30 days before we delete it.

We handle this routinely. If you have been following the recent patches, you can see many examples of it.

Placing this script inline with the HTML makes it susceptible to similar caching issues, which could cause maintenance problems down the line.

Good point. What could happen if the async JS is not the same version as the inline JS? The inline part only loads the setting and sets the checkbox state. If for ex. the localStorage item's name was changed, this wouldn't load the setting, leaving the checkbox in the default state. This can be avoided by the async JS - which saves the setting - saving with both the old and new names for a week, then the old name can be removed.
This is the same technique that we use for CSS class changes. Routine as I've said above.
That being said, we can expect this code to be very stable. It's minimal, bare-bones, not much to change. Maybe the setting name would be changed once in many years. That maintenance "burden" can be handled.

So even though I agree that the inline-script solution does provide a better user experience (making it available for anonymous users, and being device specific),

This should matter the most: creating a fully functional solution, not one constrained by the aging infrastructure and assumptions.

for the reasons mentioned above we're currently favouring limiting this feature to logged-in users only, and storing it as a user-preference.

I understand that. I hope that me doing a proper evaluation of the real consequences of using a client-side solution might help you reconsider this decision. Although I don't have high hopes, I do my best to give factual technical justification for spending a little time with investigating that solution. In the end the server-side solution will have more complications and take up more developer time, and that's not including the expect-able inflow of complaints (desktop->device transfer) and feature requests by readers.
I firmly believe much of that time can be saved with a client-side solution.

The CDN cache from the application perspective is and remains 14 days. This was lowered two years ago from the historical 31 days.

As implementation detail, one could know that our frontend and backend caches generally revalidate within 1-4 days (upto 7 days afaik). This means that if a post-edit purge got lost, or if an epoch unrelated to the page was raised, the CDN will revalidate within 7 days. However, if no edit was made, then after 7 days the internal revalidation will come back positive (304 Not Modified) and renew the cache object as-is. The max-age of the cache content is 14 days, after which renewal/revalidation would be denied.

Change 606306 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] Trigger 'change' event when checkbox state changes

https://gerrit.wikimedia.org/r/606306

Change 606307 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Persist sidebar collapsed state client-side

https://gerrit.wikimedia.org/r/606307

Change 592328 abandoned by Aron Manning:
[DNM-POC] Demo: Sidebar button (label) in header, Sidebar separately in #navigation

Reason:
Persistency implemented upon final sidebar implementation: I739c3a23e34d84488107e3d161263e65c3b6ce81
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /606307

https://gerrit.wikimedia.org/r/592328

@ovasileva did you mean to Decline this task as the acceptance criterias aren't met and there's no patch merged?
Completing_tasks: "A task is given the Resolved status when a code change that fixes the task has been merged in Gerrit. A task is given the Declined status when [...] there's a consensus that implementing a particular task would be a bad idea."

@Demian this is a spike (Spike). T246427#6234292 makes it clear there will be follow up.

Change 607164 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Add setCheckedState() function

https://gerrit.wikimedia.org/r/607164

Change 607188 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [WIP] [modern] Persist sidebar collapsed state client-side

https://gerrit.wikimedia.org/r/607188

Change 606436 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [DNM] [modern] Persist sidebar collapsed state client-side

https://gerrit.wikimedia.org/r/606436

Change 592328 restored by Aron Manning:
[DNM-POC] Demo: Sidebar button (label) in header, Sidebar separately in #navigation

https://gerrit.wikimedia.org/r/592328

Change 607188 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [WIP] [modern] Persist sidebar collapsed state client-side

https://gerrit.wikimedia.org/r/607188

Demian renamed this task from [Spike 8hrs] Make collapsible sidebar persistent across sessions for logged-in users, for sessions for logged-out users to [Spike 8hrs] Decide how to persist state of collapsible sidebar across sessions for logged-in users, logged-out users.Jul 6 2020, 1:38 AM
Demian updated the task description. (Show Details)
NOTE: This task has been closed with unreviewed patches, contradicting @Jdlrobson's statement. I have created T257169 to host the open patches uploaded starting at T246427#6184164.

[...] Resolved tasks should never have open patches associated with them. Create a new task referencing this one if more discussion is needed.

Change 610911 had a related patch set uploaded (by Jdrewniak; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Trigger 'input' event when checkbox state changes

https://gerrit.wikimedia.org/r/610911

Change 610911 merged by jenkins-bot:
[mediawiki/core@master] checkboxHack: Trigger 'input' event when checkbox state changes

https://gerrit.wikimedia.org/r/610911