Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preferences not restored on Firefox 67b4+ #992

Closed
yurikhan opened this issue Mar 27, 2019 · 19 comments
Closed

Preferences not restored on Firefox 67b4+ #992

yurikhan opened this issue Mar 27, 2019 · 19 comments

Comments

@yurikhan
Copy link

To reproduce:

  1. Start Firefox 67 beta 4 or later with a clean profile.
  2. Navigate to https://devdocs.io/.
  3. Open Preferences by clicking the link from the welcome page.
  4. Check the Enable dark theme check box. (Observed: box is checked, dark theme is applied.)
  5. Reload the page. (Observed: box is still checked but light theme is applied. Expected: box is checked, dark theme is applied.)
  6. Uncheck the Enable dark theme check box. (Observed: box is cleared, dark theme is applied. Expected: box is cleared, light theme is applied.)

Sidebar auto-hide is also affected. Did not test every preference setting.

Additional info: mozregression points to this set of commits in Firefox that has something to deal with cookies.

@tomsmeding
Copy link

For me, the only setting that doesn't seem to be applied correctly is the dark mode; the checkbox keeps its correct value, but devdocs always starts in light mode for me. Funnily, if the checkbox was ticked, then unticking it switches to dark mode!

@sbrl
Copy link

sbrl commented Apr 6, 2019

I'm seeing this too. It's really annoying!

If there's anything I can do to help out, please let me know :-)

@Carighan
Copy link

Carighan commented May 2, 2019

Same here, @tomsmeding . Everything else works but the page always starts in light mode. And same weird unticking-to-go-dark behavior, though if I reload again afterwards it's of course light mode again. 🤷‍♂

@phil-r
Copy link

phil-r commented May 12, 2019

Looks like it's cache related. Disabling cache in network tab in dev tools fixes the issue.

Maybe it's application cache(it's getting depricated)?

@sbrl
Copy link

sbrl commented May 12, 2019

You're right! It does appear to work around the issue, but as soon as I close the dev tools it breaks again :-(

Is devdocs still alive? It was a really nice resource that I use daily. This bug is a deal-breaker for me.

@jmerle
Copy link
Contributor

jmerle commented May 12, 2019

Yep, DevDocs is still alive but development is going a bit slow at the moment. We've had a meeting with all maintainers about it last week and are planning to start deploying old & new changes in the near future.

As for this issue, I know the cause but am not exactly sure why it's going wrong on Firefox (and not Chrome). I also don't seem to be able to re-produce this more than once.

Currently, the theme is defined here:

<html<%= ' manifest="/manifest.appcache"' if App.production? %> prefix="og: http://ogp.me/ns#" lang="en" class="_booting _theme-<%= app_theme %>">

The app_theme variable you see here is defined here:

@app_theme ||= memoized_cookies['dark'].nil? ? 'default' : 'dark'

The line aboves attempts to read the 'dark' cookie from the Cookie header. My best guess is that this header is not sent on Firefox (it's an optional header) but is sent on Chrome.

We can fix this by (obviously) not relying on the existence of this header. This can be done by loading the theme client-side upon loading the page. The problem is that this might cause weird effects on slow connections, since we'd default to the light theme and then show the dark theme after the JavaScript has loaded. Since it does work on Chrome, we could keep the current way of setting the theme so it stays instant for Chrome users on slow connections, and implement the client-side theme switching for users who don't sent the cookies in the HTTP request.

I'll implement the above somewhere next week.

@sbrl
Copy link

sbrl commented May 12, 2019

Ah, thanks for the update @jmerle!

Cool, that would be great! If you need help testing, I can reliably reproduce this bug every time. System information:

  • uname -a: Linux Riikaan 4.18.0-18-generic #19-Ubuntu SMP Tue Apr 2 18:13:16 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
  • Firefox version: Mozilla Firefox 67.0b17
  • lsb_release -d: Ubuntu 18.10

Also, perhaps the new prefers-color-scheme CSS media query would be another alternative? It seems to be gaining traction quickly, as both Firefox and Safari(!) support it already, with chrome support coming soon in v76.

@marienz
Copy link

marienz commented May 22, 2019

Curious. Seeing this in Firefox 67:

If I tick "Disable cache" in dev tools and reload, it works: I see a load of / with the "dark" cookie sent and class="_booting _theme-dark" in the response.

If I untick "Disable cache" and reload, the request for "/" has:

  • a "Date" response header about 3 seconds after the one from the request with cache disabled (which is well before I reloaded the page).
  • "cache" in the "transferred" column
  • the "dark" cookie supposedly set in the request header
  • class="_booting _theme-default" in the response
  • the timestamp in the response header matches an entry in about:cache?storage=appcache&context=

I can reproduce this against devdocs.io, but if I run devdocs locally the same caching does not occur (and the problem does not occur).

So my best guess (I'm not a web developer, so I may be way off the mark!) is Firefox normally does send the "Cookie" header, but issues an additional request (that I think isn't listed in the dev console) to populate the application cache, and that one lacks the header. Since I can't reproduce it against a local dev instance (and I don't have an SSL-defeating request-header-logging proxy handy) I can't quite prove that's what's going on, though.

It looks a bit similar to a firefox bug where it stopped sending cookies from requests in a service worker. That bug was a regression from the cookie changes flagged in the first comment on this issue. Given the nature of those cookie changes it definitely seems possible that Firefox stopped sending the Cookie header for some type(s) of requests, possibly unintentionally.

But since I don't actually know how either the application cache or the Cookie header or HTTP cache invalidation in general are supposed to work, I don't know if this is a Firefox or a devdocs bug :)

But even if it is a Firefox bug, there's a different cookies-and-appcache Firefox bug that got closed because "Appcache is dead"... 3 years ago :) So even if this turns out to be an unintentional Firefox change, I wouldn't count on it getting fixed there...

@jmerle
Copy link
Contributor

jmerle commented May 22, 2019

A little update from my side: some study-related tasks came up and took most of my time last week, so that's why there's no PR yet. Implementing the solution I described in my previous comment is still on my todo list though.

@marienz thanks for the information. Sounds like we should get #774 done as soon as possible so that we can get rid of the AppCache. Because switching to service workers will take a while to implement due to the amount of changes that need to be made, I will still go ahead with my solution for this issue.

@steelywing
Copy link

steelywing commented Jun 5, 2019

As for this issue, I know the cause but am not exactly sure why it's going wrong on Firefox (and not Chrome). I also don't seem to be able to re-produce this more than once.

This issue happen on both of my Firefox and Chrome (Windows)

The line aboves attempts to read the 'dark' cookie from the Cookie header. My best guess is that this header is not sent on Firefox (it's an optional header) but is sent on Chrome.

I check the dark cookie in Dev Tools, it is sent.

But I check in Fiddler, the request https://devdocs.io/ after https://devdocs.io/manifest.appcache doesn't send any cookie, so the cached page theme is _theme-default.

@steelywing
Copy link

May be use a temp solution?

Add this to init JS?

if (document.cookie.split(';').filter(function(item) {
    return item.indexOf('dark=1') >= 0
}).length) {
    var classeList = document.documentElement.classList;
    classeList.remove('_theme-default');
    classeList.add('_theme-dark');
}

@jmerle
Copy link
Contributor

jmerle commented Jun 5, 2019

There is already a PR for a non-temp solution in #1011.

@jmerle
Copy link
Contributor

jmerle commented Jul 19, 2019

The fix has been deployed, it should work correctly now.

@jmerle jmerle closed this as completed Jul 19, 2019
@sbrl
Copy link

sbrl commented Jul 20, 2019

I can verify that it does work as intended ❤️

@griest024
Copy link

I'm not seeing this change on chrome on https://devdocs.io. A couple of other fixes are also not present, including #831.

@jmerle
Copy link
Contributor

jmerle commented Jul 25, 2019

@griest024 can you try it in an incognito window? If that doesn't work, have you blocked/disabled cookies by any chance?

@griest024
Copy link

@jmerle Incognito window has the same behavior. The dark theme preference is saving, FWIW. The cookies are present and have the correct values, as far as I can tell. For example layout: _sidebar-hidden.

@sbrl
Copy link

sbrl commented Jul 28, 2019

Try clearing the cache or opening the dev tools -> disable cache?

@griest024
Copy link

@sbrl yeah I had already tried that...still didn't work :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants