Fixed: Add sameSite attribute to Backdrop tableDrag cookie.

[indigoxela]

When visiting a page with tabledrag, Firefox nags into the console:

Cookie “Backdrop.tableDrag.showWeight” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute.
More information about that nagging: https://developer.mozilla.org/de/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Backdrop uses jquery.cookie.js

Actually using a cookie for tabledrag is not necessary it just toggles display stuff via js. I'd recommend to use localStorage instead.

This is a related Drupal 8 issue, where they switched to localStorage as well.

[laryn]

Related?
https://www.zoocha.com/news/how-fix-chrome-80-cookies-issue-drupal

[indigoxela]

Related?

Yep, seems to be related.

I have to admit, that I'm really no expert regarding that topic.

Another thing I just figured out: Backdrop ships with jquery.cookie.js version 1.0 (2006), but that's pretty outdated:
The initial project has been archived and replaced by a new project js-cookie.

It seems that sameSite attribute support has been added in 2016: js-cookie/js-cookie#276

We should really update jquery.cookie.js. (or actually switch to js.cookie)

[quicksketch]

Considering js-cookie and jquery.cookie.js were written by the same author, they helpfully provided a backwards-compatible approach that we might be able to use: https://github.com/js-cookie/js-cookie/tree/v1.5.1#migrating-from-jquery-cookie

$.cookie('name', 'value') === Cookies.set('name', 'value')
$.cookie('name') === Cookies.get('name')
$.removeCookie('name') === Cookies.remove('name')
$.cookie() === Cookies.get()

Though I'm not sure if that's really a good idea. Backdrop core's usage of the library is pretty minimal though, only in Tabledrag, Contact module, and Comment module.

[indigoxela]

Maybe this D8 issue is useful here?

[indigoxela]

Regarding tabledrag.js and the annoying cookie console log... Heck, why don't we simply switch to localstorage? There's actually no need for a cookie (which always gets sent via HTTP), because the switching happens in js in the client, anyway. Or am I missing something?

Examples:

localStorage.getItem('Backdrop.tableDrag.showWeight');
localStorage.setItem('Backdrop.tableDrag.showWeight', '1');

... as simple as that. 😁

[indigoxela]

Coming back here after some time. A PR is available for testing and review.

How to test:

• Go to a page with tabledrag, like /admin/structure/types/manage/post/fields or /admin/structure/menu/manage/main-menu
• Toggle the "Show row weights" setting
• Check if toggle on/off works as expected

If you're more curious what happens, open the browser dev tools an check Web storage / Local storage for the toggling value of Backdrop.tableDrag.showWeight.
Note that a cookie with the same name might still exist, if you test locally on a Backdrop you logged in before, but it's not in use anymore and doesn't cause anymore console nagging.

[docwilmot]

I believe this looks good. If it works for the Firefox users I'd RTBC it, but cant test without FF.

[herbdool]

I've tested in FF. Code looks good.

Should we have follow-up issues for changing the cookie library in Contact and Comment as well?

[klonos]

Although the PR by @indigoxela works fine and it is an improvement, I'd be more comfortable if we followed Drupal changes more closely where possible, because:

• Drupal has a bigger user base, so things get more real-life testing
• If security/performance fixes come out for Drupal, it'll be harder for us the more we have deviated.

Also, from https://www.drupal.org/project/drupal/issues/1569648 that you have linked @indigoxela

Also using localStorage mean we can listen to events and react in background tabs. Open a "manage fields" page for some content type, then open another tab/window in the same or another "manage fields" page. Click "Show row weight" in one tab and look at the other one, the change propagated.

I have tested the above in the now "Manage displays" tab in D8/9, and it does indeed work w/o needing to refresh the page. That doesn't happen in the PR that's been RTBCed here.

With all respect to your solution @indigoxela, here's an alternative PR for consideration, which mimics the changes that went into Drupal: backdrop/backdrop#3870

[klonos]

Should we have follow-up issues for changing the cookie library in Contact and Comment as well?

I think so, yes 👍🏼 ...here's a follow-up: #5407

I also think we need a follow-up to backport other fixes that have been committed to tabledrag.js over the past 8-9 years: #5408

[indigoxela]

I've updated my PR to not set a value by default, which is more closely to what D8 did - and this is the part that makes sense to me. 😉

As we now have a competing PR here, IMO it makes sense to go back to "needs testing/review".

@klonos I left a hint on your PR re the deprecated "proxy" and "bind" functions.

[herbdool]

@indigoxela I've reviewed and tested your PR. Looks good.

[klonos]

Sorry guys. I would have made code snippet suggestions (you know I always do), but the code seemed totally different, so that wasn't possible/easy.

I still think that there's value in keeping code parity with Drupal commits where possible (as well as feature parity), as I think that it makes back/cross-ports easier even for less experienced developers, but happy to close my PR if there's no consensus re that. If the toggle propagation to all active tabs is requested by users in the future, I personally won't be able to contribute - we'd need someone with JS expertise to do that for us in a custom way (as opposed to the ready solution we have from the Drupal commit).

Having said all that, I understand the concerns re introducing deprecated functions, and I have now removed usage of .proxy() from my PR.

[indigoxela]

@klonos This throws the issue back to "needs more feedback". You seem to insist in your solution, which undermines the current status - it was already RTBC. I don't see any chance to solve this conflict without the PMC.

No, I won't close my PR. You won't close yours, I assume. That means that an easy fix gets stuck.

[klonos]

I don't think that this issue needs to go to the PMC. We have 4 active core committers, so they can decide what is the best way forward.

We have a unique solution, which works fine, and we have an alternative, which also works fine. There were concerns with the alternative solution (which is basically a back/cross-port of the Drupal solution), which I have now addressed (no more usage of deprecated jQuery functions). Both are valid the way I see it - we just need to let the core committers decide which is the most appropriate way to solve this. This is nothing new, so no dramas.

[quicksketch]

I read both PRs and personally I find @indigoxela's approach to be easier to read and understand. Despite the benefit of matching Drupal code, I would prefer to go with @indigoxela's approach because it may be easier for our own community to maintain it independently.

In testing, I found that both approaches immediately update the columns across tabs (which I find unexpected, but in some cases could actually be useful). Since it's the same in both, that probably doesn't need to be part of the consideration as to which we should use.

[klonos]

Thanks @quicksketch 👍🏼 ...I've closed my PR then.

[quicksketch]

Merged backdrop/backdrop#3869. Thanks everyone for the work here. Major props to @indigoxela for her solution and thanks to @klonos for being gracious in coming to an agreement. And thanks to reviewers @herbdool, @argiepiano, and @docwilmot. I'm really glad when our community can work out these minor conflicts. Thank you all! 🙇