1400805 - Allow WebExtensions to get/set "browser.display.use_document_fonts"

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[Fithu]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:3.2) Goanna/20170821 PaleMoon/27.4.0
Build ID: 20170710123658

Steps to reproduce:

Until now I had used the old extension "Document font toggle" (https://github.com/7adietri/document-font-toggle/blob/master/LICENSE). But it is an old addon type.

With this addon we were able to switch the setting "browser.display.use_document_fonts" with only one click.

As in my system (Windows 7) I have deactivated all antialiasing and smoothing functions for fonts, there are a lot of websites with fonts who are not suitable for using without antialising and smoothing. This is why I want to use the font from firefox instead of the font from the website. But I want to have also the possibility to quick switch to the web font.

As now with the new web-extensions, by default it is not possible to change the settings in about:config, I would like to suggest you, to allow the setting browser.display.use_document_fonts for web-exdtensions. 


Actual results:

Web-extensions do not have the right to manipulate "browser.display.use_document_fonts".


Expected results:

I would like to suggest you, to allow the setting browser.display.use_document_fonts for web-exdtensions.

Comment 1

[Bob Silverberg [:bsilverberg]]

David, a developer is requesting access to the pref "browser.display.use_document_fonts" via a WebExtensions API. We have a means for doing this, which we have used for a number of prefs already, but I want to make sure that exposing this pref makes sense and is a logical thing to do. I gather that you are involved with the module that makes use of this pref, hence my needinfoing you. 

Is this a pref that we expect to continue to exist long-term, and does allowing an extension to modify this in order to enable and disable document fonts on the fly make sense?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that seems reasonable, although we should probably double-check with Manish from a Servo point of view.

It's one of the prefs that we expose in the prefs UI (General -> Language & Appearance heading -> Fonts & Colors subheading -> Advanced button -> Allow pages to choose their own fonts, instead of your selections above).  Given that I guess it seems reasonable to me to expose to extensions, although I'm not sure what the general criteria for exposing prefs to extensions are.

Comment 3

[Manish Goregaokar [:manishearth]]

> although we should probably double-check with Manish from a Servo point of view.

We handle that pref in http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/layout/style/nsRuleNode.cpp#422-423

Is there any other place we should be dealing with it?

Comment 4

[Andy McKay]

Bob, we should ensure then we have matching (and probably blocking bug) for the about:preferences page as well.

Comment 5

[Bob Silverberg [:bsilverberg]]

Thanks David and Manish, that sounds like a "go ahead" from the two of you from your side. Andy, I opened bug 1401177 as the Project Jazz tracker. I don't think we need a design-decision-needed discussion for this one, do you Andy?

Comment 6

[Andy McKay]

No, I think we are good.

Comment 7

[Bob Silverberg [:bsilverberg]]

Thanks Andy. As this is a P5 it may be some time before it is addressed, but it is available to be worked on by any volunteer who may be interested in doing so.

Comment 8

[theDeadDuck]

Why not protecting ONLY those prefs which are crucial to security and System-Stability 
and allow the WebExtensions API to access the rest ?

Why not allowing the WebExtensions API to access those prefs which have something to do with a better user experience ?

for example:

    font.default.*
    font.name.monospace.* 
    font.name.sans-serif.* 
    font.name.serif.*
    font.system.whitelist
    browser.display.use_document_fonts


Do we really need to go through this just to enable/disable document fonts or to set the default fonts ?

(General >> Language & Appearance heading >> Fonts & Colors subheading >> Advanced button >> Select default fonts / Allow pages to choose their own font)
This is an example of user experience at its worst.

Comment 9

[Tim Nguyen :ntim]

This should be really similar to bug 1417810 to fix.

Comment 10

[Sören Hentzschel]

I would like to try it on the weekend as my first contribution to Firefox.

Comment 11

[Caitlin Neiman [:caitmuenster]]

Thanks, Soren! :zombie will mentor this bug. You might also want to refer to  https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.

Comment 12

[Sören Hentzschel]

Attachment: Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

Review commit: https://reviewboard.mozilla.org/r/233816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233816/

Comment 13

[Sören Hentzschel]

Thank you, Caitlin. I already had some time, so I did the changes and submitted a mozreview request.

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

https://reviewboard.mozilla.org/r/233816/#review239500

::: toolkit/components/extensions/parent/ext-browserSettings.js:463
(Diff revision 1)
>              },
>            }
>          ),
> +        useDocumentFonts: Object.assign(
> +          getSettingsAPI(
> +            extension, "useDocumentFonts",

The other property is called "overrideDocumentColors". I don't have much preference over "use" vs "override" but it would be nice to stay consistent :)

::: toolkit/components/extensions/parent/ext-browserSettings.js:465
(Diff revision 1)
> +              let prefValue = Services.prefs.getIntPref("browser.display.use_document_fonts");
> +              if (prefValue === 0) {
> +                return "never";
> +              } else if (prefValue === 2) {
> +                return "always";
> +              }
> +              return "allow_pages_own_fonts";

The value `2` was deprecated according to http://kb.mozillazine.org/About:config_entries#Browser.

I did a search on DXR for the pref, and there isn't any special handling for the value 2, which seems to confirm this.

The source code only handles values 1 (use document fonts) & 0 (never use document fonts).

Comment 15

[Sören Hentzschel]

Attachment: addressed review comment

Review commit: https://reviewboard.mozilla.org/r/233954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233954/

Comment 16

[Sören Hentzschel]

Thank you, Tim.

> The other property is called "overrideDocumentColors". I don't have much preference over "use" vs "override" but it would be nice to stay consistent :)

Most settings in browserSettings don't use the term "override" so I don't think this would add much consistency. For "overrideDocumentColors" the term "override" is also used in the preferences UI so it's a good choice, but for this setting the preferences UI says "Allow pages to choose their own fonts, instead of your selections above", so I think the term "use" is better than "override" for this case. That's why I didn't change that, but if you say something like "overrideDocumentFonts" is better I will change that. 

> The value `2` was deprecated according to http://kb.mozillazine.org/About:config_entries#Browser.
> I did a search on DXR for the pref, and there isn't any special handling for the value 2, which seems to confirm this.
> The source code only handles values 1 (use document fonts) & 0 (never use document fonts).

I removed support for the value '2'.

Comment 17

[Tomislav Jovanovic :zombie]

Thanks Sören, please flag me for final review once you get r+ from :ntim.

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

https://reviewboard.mozilla.org/r/233816/#review239640

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8965237 [details]
addressed review comment

https://reviewboard.mozilla.org/r/233954/#review240482

Thanks for addressing my comments! Can you combine both revisions in one single commit as well ?

::: toolkit/components/extensions/parent/ext-browserSettings.js:469
(Diff revision 1)
>                if (prefValue === 0) {
>                  return "never";
> -              } else if (prefValue === 2) {
> -                return "always";
>                }
> -              return "allow_pages_own_fonts";
> +              return "use";

"always" would be more consistent with the other prefs. Is there a reason not to use it here ?

Comment 20

[Tim Nguyen :ntim]

(In reply to Sören Hentzschel from comment #16)
> Thank you, Tim.
> 
> > The other property is called "overrideDocumentColors". I don't have much preference over "use" vs "override" but it would be nice to stay consistent :)
> 
> Most settings in browserSettings don't use the term "override" so I don't
> think this would add much consistency. For "overrideDocumentColors" the term
> "override" is also used in the preferences UI so it's a good choice, but for
> this setting the preferences UI says "Allow pages to choose their own fonts,
> instead of your selections above", so I think the term "use" is better than
> "override" for this case. That's why I didn't change that, but if you say
> something like "overrideDocumentFonts" is better I will change that. 

I personally do prefer consistency API-wise (how it's exposed in about:preferences is just an internal detail), but I'll defer to :zombie for his opinion on this.

Comment 21

[Sören Hentzschel]

> "always" would be more consistent with the other prefs. Is there a reason not to use it here ?

No, there is no reason. Naming things is really the hardest part in development. ;-) I will change that.

> Can you combine both revisions in one single commit as well ?

I am not very experienced with mercurial. What is the easiest way to combine both in one single commit?

Thank you!

Comment 22

[Tomislav Jovanovic :zombie]

> I am not very experienced with mercurial. What is the easiest way to combine
> both in one single commit?

You can use `hg histedit`, which is very similar to git interactive rebase, if you are familiar with that.  For example, if you have two commits that you want to squash into one, do:

  hg histedit -r -2

and follow instruction in the comments.

Comment 23

[Sören Hentzschel]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233816/diff/1-2/

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

https://reviewboard.mozilla.org/r/233816/#review241616

Looks good, thanks! I'll leave the naming (override vs. use) to zombie's opinion.

Comment 25

[Tomislav Jovanovic :zombie]

(In reply to Tim Nguyen :ntim from comment #20)
> I personally do prefer consistency API-wise (how it's exposed in
> about:preferences is just an internal detail), but I'll defer to :zombie for
> his opinion on this.

I too like API consistency, but how it is exposed in user-facing UI is the opposite of "internal detail". ;)

So let's go with "use", perhaps even "allow" if it sounds better.

Comment 26

[Tomislav Jovanovic :zombie]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

https://reviewboard.mozilla.org/r/233816/#review242260

Thanks, the ode looks good, but I think we should make this a boolean setting.

::: toolkit/components/extensions/parent/ext-browserSettings.js:474
(Diff revision 2)
> +              return "always";
> +            }
> +          ),
> +          {
> +            set: details => {
> +              if (!["always", "never"].includes(details.value)) {

If there are only two options, this should really be a boolean setting instead of a string enum.

Comment 27

[Tomislav Jovanovic :zombie]

> the ode looks good, 

And the code is well written too.

Comment 28

[Sören Hentzschel]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233816/diff/2-3/

Comment 29

[Tomislav Jovanovic :zombie]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

https://reviewboard.mozilla.org/r/233816/#review243006

::: toolkit/components/extensions/parent/ext-browserSettings.js:466
(Diff revision 3)
> +        useDocumentFonts: Object.assign(
> +          getSettingsAPI(
> +            extension, "useDocumentFonts",
> +            () => {
> +              let prefValue = Services.prefs.getIntPref("browser.display.use_document_fonts");
> +              if (prefValue === 0) {

nit: you can simply `return prefValue !== 0` or even `return !!prefValue`;

::: toolkit/components/extensions/parent/ext-browserSettings.js:478
(Diff revision 3)
> +            set: details => {
> +              if (typeof details.value !== "boolean") {
> +                throw new ExtensionError(
> +                  `${details.value} is not a valid value for useDocumentFonts.`);
> +              }
> +              let prefValue = 1; // initialize to 1 - allow pages to choose their own fonts

nit: and similarly: `Number(details.value)` here.

Comment 30

[Sören Hentzschel]

Comment on attachment 8965133 [details]
Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233816/diff/3-4/

Comment 31

[Sören Hentzschel]

Thank you!

I fixed the nits and used `return prefValue !== 0` because I think it's easier to read than `return !!prefValue` (and I avoided the variable because with your suggested changes I don't do anything with the variable aside from returning the values).

Comment 32

[Tomislav Jovanovic :zombie]

Thank you for fixing it.  ;)

Please mark the remaining issues on reviewboard as resolved, and add a checkin-needed flag here.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10697d111e50
added WebExtension API to get/set browser.display.use_document_fonts r=ntim,zombie

Comment 34

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/10697d111e50

Comment 35

[Caitlin Neiman [:caitmuenster]]

\o/ Awesome, Sören! This has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#April_2018

Thanks so much for jumping in. :)

Comment 36

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 37

[Will Bamberg [:wbamberg]]

Docs -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/useDocumentFonts

Please let me know if you need anything else.

Comment 38

[Tim Nguyen :ntim]

(In reply to Will Bamberg [:wbamberg] from comment #37)
> Docs ->
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/
> browserSettings/useDocumentFonts
> 
> Please let me know if you need anything else.

Shouldn't it be the opposite ?


    true: use the fonts specified by the web page. This is the default.
    false:  use the system fonts.

Comment 39

[Will Bamberg [:wbamberg]]

Oops! Fixed.

Comment 40

[Bob Silverberg [:bsilverberg]]

LGTM, thanks Will.

Comment 41

[Pixievolt No. 1]

This was originally described on https://developer.mozilla.org/en-US/Firefox/Releases/61 as "You can now set your add-on to use the same fonts as the current document using the browserSettings.useDocumentFonts property", which doesn't sound anything like what this actually does. I changed it to "You can now force web pages to use system fonts instead of the fonts they specify using the browserSettings.useDocumentFonts property".

Comment 42

[Pixievolt No. 1]

Why is Bugzilla saying I changed "status-firefox57"? I didn't even see any options for changing any properties of that nature...

Comment 43

[Will Bamberg [:wbamberg]]

Thanks for the fix pikadudeno1!