Fix #94 Move static prefs to mobile-config-prefs.js

added 1 commit

fa8c349e - Fix #94 Move prefs to mobile-config-autoconfig.js

The only exceptions are the prefs to enable autoconfig.js and userChrome.css to avoid accidental breakage of our config.

This makes sense! Can you group them together and add a comment that says this on top?

Static preferences should now reside in mobile-config-prefs.js and use defaultPref to allow the user to override them.

Why are there defaultPref() added to mobile-config-autoconfig.js in this patch, was this done by accident?

In general, I'm wondering why some options are in both files now. Shouldn't it be enough if they are in one file?

Thanks for the patch @dannycolin!

...marking as draft, please un-mark this as draft when it is ready for another round of review.

Why are there defaultPref() added to mobile-config-autoconfig.js in this patch, was this done by accident?

Other preferences in *-autoconfig.js use defaultPref. I was under the impression that it would be better to do the same with the moved preferences. This way, it's possible for the user to change the value permanently.

Rethinking about it, we might want to prevent the user to change some of them. I'll revert them to pref for this patch but we can revisit the idea of using defaultPref later on.

Ah sorry, I've missed when writing my previous comment that defaultPref allows overriding the preference by the user and pref does not.

With that in mind, I think we should use defaultPref for all of them, so the user can override them if they want to. Or is there any reason we should not do that?

So basically removing the last patch that changes defaultPref -> pref, and adjusting the remaining "pref" to "defaultPref" in mobile-config-autoconfig.js:

+    // Allow UI customizations with userChrome.css and userContent.css
+    pref('toolkit.legacyUserProfileCustomizations.stylesheets', true);

+    // Enable android-style pinch-to-zoom
+    pref('dom.w3c.touch_events.enabled', true);
+    pref('apz.allow_zooming', true);
+    pref('apz.allow_double_tap_zooming', true);

Otherwise the diff looks good to me now (I'd squash the commits into one, but I can do that before merge). I haven't tested it yet though.

Could you make the adjustments if they make sense?

As I said, a few prefs might need to use pref so the user doesn't accidentally disable mobile-config-firefox or an important pref needed to load our modifications. Some prefs are in a grey area regarding if they need to be locked.

What I can do is only using pref for the preferences that are critical and use defaultPref for everything else.

pref doesn't prevent people from changing the value but will reset it on every start. In my experience this will lead to confused users.

So I would recommend to ether use defaultPref or lockPref.

@sertonix The idea is to prevent users to accidentally turn off only 2 or 3 prefs that are needed to load mobile-config-firefox. However, I'd still want to give someone a way to disable it from a user.js file if they need to debug something. IIRC, this isn't possible with lockPref but I'll double check.

@ollieparanoid I used defaultPref for all the preferences for now because of the concern raised by @sertonix. We can always revisit this later on.

marked this merge request as draft

added 1 commit

05e21f36 - remove moved prefs in mobile-config-prefs.js

Compare with previous version

added 1 commit

a9809469 - revert defaultPref to pref for the moved prefs

Compare with previous version

marked this merge request as ready

added status::mr-stale label

Sorry to bother you @dannycolin,

but we've detected that this merge request hasn't seen any recent activity. If you need help, want to discuss your approach with developers, or you are all done and this is waiting for review, you can ping @postmarketOS. You can also ask on matrix in #devel:postmarketos.org or #postmarketos-devel on OFTC.

Thanks for your contribution.

removed status::mr-stale label

mentioned in issue #100 (closed)

added 1 commit

e919d08e - Use defaultPref for all preferences

Compare with previous version

I've tried it out with firefox-esr-128.8.0-r0 and unfortunately with this MR, there is a visual regression with the tab bar size. I guess one or more option doesn't apply that way?

Could you set up an environment to reproduce this and fix it (i.e. move the option(s) back that cannot be set this way?)

If you currently can't test it on a phone, you could also spin up a VM with pmbootstrap: https://postmarketos.org/qemu

current master:

this merge request:

I've looked into this (in the dumb way that my mind came up with) by reviewing what the settings in about:config look like after applying mobile-config-firefox:

With this MR: Everything is set as it is declared, only browser.newtabpage.enabled', false is not, it is set to true.

With current git master:

browser.newtabpage.enabled', false is also not set, it is set to true.

and, more importantly: browser.uidensity', 2 is also not set, it is set to 0.

So, IMHO, this is not Danny's bug, it's mine, gnumdks, or, everybody's bug, see !57 (diffs).

Since we need to touch tab-width anyway to not have a serious regression in what we set as default tab behavior with current/next ESR, I think we should instead decide with which value browser.uidensity actually works better in general across Firefox. Alternatively, if we don't want to make a decision, we could see if we can solve this with via CSS media query.

Edit: !74 (merged) adds support for uidensity=touch.

@1peter10 Can you try with the pref() instead of defaultPref() for the preferences that aren't honoured? I'm curious if it isn't a problem related to the order in which pref files are loaded by Firefox. If that's the case, it could explain why using defaultPref() doesn't work for some preferences.

I am not particularily familiar and don't understand much about this part, and have a few userChrome-improvements/fixes cooking that I want to get MR ready. .. So while I can look into this, it won't be soon - other projects need my scarce time, too.

Thanks to both of you for looking into it! in the meeting we discussed to just keep browser.uidensity with the old method (mobile-config-prefs.js) to avoid this regression and move everything else over.

mentioned in merge request !73 (merged)

Fix #94 Move static prefs to mobile-config-prefs.js

Merge request reports

Activity