Page MenuHomePhabricator

Use localstorage for sitenotices instead of cookies
Closed, ResolvedPublic

Description

This still uses cookies instead of mw.storage (local/sessionstorage) and should be converted.

Event Timeline

TheDJ created this task.Jul 10 2018, 8:38 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2018, 8:38 PM

Change 447404 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/DismissableSiteNotice@master] Use local storage instead of cookies for site notices

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

D3r1ck01 triaged this task as Normal priority.Jul 23 2018, 12:44 PM
TheDJ added a comment.Jul 23 2018, 2:25 PM

Note that I use session storage as a backup, for privacy mode and cookie blocking browsers/extensions. Dismissible notice is less used, so maybe it isn't important there. Not sure how often dismissible site notice is used to begin with if im honest.

That makes sense @TheDJ. Thanks and as for whether to use session storage as a backup, I'm not quite sure (no clue). But I've fixed the patch to keep using cookies for <= 30 days before moving completely to local storage. :)

Nirmos added a subscriber: Nirmos.Jul 24 2018, 4:01 PM

Patch Set 4: Code-Review-1
Please check the note on T110353, with regards to not having a way to expire the data, which means it builds up indefinitely. We may want to consider mw.storage.session instead if the shorter expiry is acceptable (note that sessionStorage in browsers can actually persist as long as 30 days or longer even, given that browsers tend to resume previous sessions even upon restarts).
Alternatively, we could make an exception given it's only a single key, but it's not great given that'll be what every extension does for their "one" small key/value, also with regards to site admins removing the extension and the data remaining permanently.

@Krinkle, moving discussion from Gerrit here;
I've got a few questions @Krinkle that will better my understanding of the issue;

  • Do we need that when a user dismisses a site notice, it should resurface again after a while? If so, is there a standard duration? e.g. 30 days for example?
  • localStorage is almost same as sessionStorage and using the former, it keeps data indefinitely but the later can of-course expire, so should we go for it?
  • Why should a user dismiss a site notice and it appears again after a while? Would love to know the reasoning behind this.

Thanks for the review and once I get your feedback, I'll continue work on the patch :)

@D3r1ck01 The reason for expiration is not about seeing the same notice again. It is about not storing data longer than needed. If wiki A creates a notice "1" and enables it, and then disables it after a few weeks, and wiki A does not create another notice for 3 years, then we do not need to store forever that you've seen "notice 1".

Also, if you do not visit a wiki very often, we should allow the browser to delete the data at some point.

You can multiply this with the number of wikis you visit, and multiply with the number of features that exist that can toggle something (table of contents, watchlist notice, site notice, WikiEditor toolbars, "templates used on this page" edit page, etc. etc.). Also keep in mind that gadgets can also create this kind of functionality, and gadgets are sometimes removed, abandoned or disabled. This again causes more data to stay around indefinitely. To prevent the storage from getting full (it is limited to 5 MB in total), we should avoid storing data without an expiry.

The DismissableSiteNotice extension currently uses 30 days for storing the data (in cookies). This is a very common default and works well. However, unfortunately, localStorage does not have a way to expire data. So our only choice is:

  • Keep "forever".
  • Keep for the session.

Given that DismissableSiteNotice only stores 1 piece of data, and is an extension that tends to be installed always, it is probably safe to use "forever" without wasting data (given the only way for the data to be redundant is if the wiki is not visited often, or if the extension is uninstalled).

On the other hand, perhaps sessionStorage is good enough because it seems sessions can survive quite long, perhaps even longer than 30 days?

Thanks so much for the explanation @Krinkle. While on one hand, cookie expiration time can be explicitly set (in the previous case 30 days) in the source code and we no longer need this solution, on the other hand, sessions can really persist as long as the browser is open, can also survive page reloads and restores. If a wiki page is opened in a new tab or window, a new session will be initiated so this means closing the window or tab will expire the previous session (and is this something we want to do?). See: https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API.

Since we do not need to keep this indefinitely as the extension maybe uninstalled etc (as you've already mentioned above), I think a reasonable solution here will be using sessionStorage. In this light, I'll update the patch and submit a PS :)

@D3r1ck01 Sounds good. If you're interested in experimenting with this, I've got a basic demo at CodePen that might help.

@D3r1ck01 Sounds good. If you're interested in experimenting with this, I've got a basic demo at CodePen that might help.

Thanks a lot, I did experiment on this locally after fixing the patch. So it looks good now :), up to you for review :)

D3r1ck01 moved this task from Backlog to Doing [WIP] on the User-D3r1ck01 board.

Change 447404 merged by jenkins-bot:
[mediawiki/extensions/DismissableSiteNotice@master] Use session storage instead of cookies for site notices

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

D3r1ck01 closed this task as Resolved.Aug 20 2018, 9:31 AM

Users on nlwiki are complaining they can't hide the sitenotice: https://nl.wikipedia.org/wiki/Wikipedia:De_kroeg#Suriname_en_de_Caraïben

I don't see dismissSiteNotice appearing in my local storage after dismissing the sitenotice.

Users on nlwiki are complaining they can't hide the sitenotice: https://nl.wikipedia.org/wiki/Wikipedia:De_kroeg#Suriname_en_de_Caraïben

I don't see dismissSiteNotice appearing in my local storage after dismissing the sitenotice.

Look in sessionStorage instead. It works for me?

  1. View https://nl.wikipedia.org/wiki/Wikipedia:De_kroeg, click "Sluiten"
  2. Reload the page. Banner does not appear.
In the console
sessionStorage

// shows:
Storage {dismissSiteNotice: "2.68", ... }
Sjoerddebruin added a comment.EditedSep 2 2018, 7:05 AM

Users on nlwiki are complaining they can't hide the sitenotice: https://nl.wikipedia.org/wiki/Wikipedia:De_kroeg#Suriname_en_de_Caraïben

I don't see dismissSiteNotice appearing in my local storage after dismissing the sitenotice.

Look in sessionStorage instead. It works for me?

  1. View https://nl.wikipedia.org/wiki/Wikipedia:De_kroeg, click "Sluiten"
  2. Reload the page. Banner does not appear.

    `name=In the console sessionStorage

    // shows: Storage {dismissSiteNotice: "2.68", ... } `

It does add it to sessionStorage and it doesn't appear again when you normally refresh or navigate. However, if you open a new page (in a tab), it appears again and the entry in sessionStorage is gone. Using Safari 11.1.2 on Mac OS 10.13.6.

Edit: I know read that this is intended behaviour for sessionStorage, quite annoying.

Gryllida added a subscriber: Gryllida.EditedSep 3 2018, 1:55 AM

Hello all,

We have the same issue here about opening the page in a new tab: the site notice re-appears. This worked correctly previously: the site notice would normally remain dismissed if the user dismissed it in the previous tab. Please if you could fix it, it would be great for saving the precious screen space and increasing productivity.

Gryllida

EDIT: I can reproduce this issue in Google Chrome 68.0.3440.106 (Official Build) (64-bit) on Windows 7.

EDIT: also reproducible in Firefox

Gryllida rescinded a token.
Gryllida awarded a token.
Gryllida rescinded a token.
Gryllida awarded a token.

Change 457515 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/DismissableSiteNotice@master] Revert "Use session storage instead of cookies for site notices"

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

Change 457516 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/DismissableSiteNotice@wmf/1.32.0-wmf.19] Revert "Use session storage instead of cookies for site notices"

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

Change 457515 merged by jenkins-bot:
[mediawiki/extensions/DismissableSiteNotice@master] Revert "Use session storage instead of cookies for site notices"

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

Change 457516 merged by jenkins-bot:
[mediawiki/extensions/DismissableSiteNotice@wmf/1.32.0-wmf.19] Revert "Use session storage instead of cookies for site notices"

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

Mentioned in SAL (#wikimedia-operations) [2018-09-04T11:19:52Z] <hashar@deploy1001> Synchronized php-1.32.0-wmf.19/extensions/DismissableSiteNotice: Revert "Use session storage instead of cookies for site notices" - T199274 (duration: 00m 50s)

D3r1ck01 added a comment.EditedSep 4 2018, 11:24 AM

@Gryllida, @Sjoerddebruin, we've reverted the change from "session storage" to "cookie storage" (via a SWAT deploy window) to mitigate the issue you've reported. Thanks! Now we should have cookies running around again :). See this: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DismissableSiteNotice/+/457516.