stoically / temporary-containers

Firefox Add-on that lets you open automatically managed disposable containers
https://addons.mozilla.org/firefox/addon/temporary-containers/
MIT License
852 stars 60 forks source link

options.html can get out of sync with background preferences #433

Closed rkodey closed 4 years ago

rkodey commented 4 years ago

Actual behavior

I believe the in-memory preferences within options.html are getting out of sync with the background extension preferences. The result is that in preferences.ts the handleChanges method sees differences in oldPreferences vs newPreferences that aren't really there, resulting in unexpected preference changes.

Expected behavior

Perhaps force-reload preferences into options.html when needed. I notice that when isolation is toggled from the popup, it forces a reload of options.html. Perhaps I can trigger the same in my new handler replacement for the commands.ts toggle isolation. That way, both Alt-I and the new autoEnableIsolation can benefit.

Steps to reproduce

  1. Open Preferences / Options (in my case that opens into a tab)
  2. Go to Advanced, and make sure Alt-I isolation toggle is enabled
  3. Do not close the Preferences / Options tab
  4. Go to any other tab, or open a new tab, or even stay on the Preferences / Options Tab
  5. Press Alt-I to toggle isolation
  6. Go back to the Preferences / Options tab
  7. Click on any Checkbox preference anywhere in Preferences
  8. The Isolation active state will toggle back the instant you click any checkbox on the page

Notes

I encountered this behavior while testing my changes for PR #429. I re-tested on my main (non-dev) Firefox profile using the current production Temp Containers, and see the same behavior there. So, I think/hope I didn't introduce this behavior! Granted, it's a bit convoluted and requires the options.html to be open while other toggles are changed.

What I think is happening: Anything triggered from commands.ts that updates preferences can put an existing options.html out of sync. Perhaps the isolation toggle is the only preference change that happens outside of options.html?

rkodey commented 4 years ago

Actually, there's a pretty good argument to move preferences.isolation.active out of preferences and instead persist it in local storage instead. That would make it related and similar to the proposed change here: https://github.com/stoically/temporary-containers/pull/429#pullrequestreview-434625290

stoically commented 4 years ago

Good catch!

Perhaps the isolation toggle is the only preference change that happens outside of options.html?

Yeah I think that's currently the only code path where this could happen. I think it also affects the popup - like if the popup is open and the command fires the popup won't refresh the preferences.