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

RESOLVED FIXED in Firefox 61

Status

P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: Pawihe, Assigned: soeren.hentzschel, Mentored)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, good-first-bug})

unspecified
mozilla61
dev-doc-complete, good-first-bug
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [browserSettings] triaged)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Component: General → WebExtensions: General
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?
Flags: needinfo?(dbaron)

Updated

a year ago
status-firefox57: --- → wontfix
Priority: -- → P5
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.
Flags: needinfo?(dbaron) → needinfo?(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?
Flags: needinfo?(manishearth)

Comment 4

a year ago
Bob, we should ensure then we have matching (and probably blocking bug) for the about:preferences page as well.
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?
Flags: needinfo?(amckay)

Comment 6

a year ago
No, I think we are good.
Flags: needinfo?(amckay)
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.
Blocks: 1363856
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [browserSettings] triaged
Comment hidden (advocacy)
This should be really similar to bug 1417810 to fix.
Keywords: good-first-bug
Summary: making available "browser.display.use_document_fonts" for webextensions → Allow WebExtensions to get/set "browser.display.use_document_fonts"
(Assignee)

Comment 10

8 months ago
I would like to try it on the weekend as my first contribution to Firefox.
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.
Mentor: tomica
Comment hidden (mozreview-request)
(Assignee)

Comment 13

7 months ago
Thank you, Caitlin. I already had some time, so I did the changes and submitted a mozreview request.
(Assignee)

Updated

7 months ago
Assignee: nobody → cadeyrn

Comment 14

7 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 16

7 months ago
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'.
(Assignee)

Updated

7 months ago
Attachment #8965237 - Flags: review?(ntim.bugs)
Thanks Sören, please flag me for final review once you get r+ from :ntim.
Mentor: tomica → ntim.bugs

Comment 18

7 months ago
mozreview-review
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
Attachment #8965133 - Flags: review?(tomica)

Comment 19

7 months ago
mozreview-review
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 ?
Attachment #8965237 - Flags: review?(ntim.bugs)
(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.
(Assignee)

Comment 21

7 months ago
> "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!
> 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 hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8965237 - Attachment is obsolete: true

Comment 24

7 months ago
mozreview-review
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.
Attachment #8965133 - Flags: review+
(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

7 months ago
mozreview-review
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.
Attachment #8965133 - Flags: review?(tomica)
> the ode looks good, 

And the code is well written too.
Comment hidden (mozreview-request)

Comment 29

7 months ago
mozreview-review
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.
Attachment #8965133 - Flags: review?(tomica) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 31

7 months ago
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).
Thank you for fixing it.  ;)

Please mark the remaining issues on reviewboard as resolved, and add a checkin-needed flag here.
Flags: needinfo?(cadeyrn)
(Assignee)

Updated

7 months ago
Flags: needinfo?(cadeyrn)
Keywords: checkin-needed

Comment 33

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/10697d111e50
added WebExtension API to get/set browser.display.use_document_fonts r=ntim,zombie
Keywords: checkin-needed

Comment 34

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10697d111e50
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Updated

7 months ago
Keywords: dev-doc-needed
\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

7 months ago
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.
Flags: needinfo?(cadeyrn)
Flags: needinfo?(cadeyrn) → qe-verify-
Docs -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/useDocumentFonts

Please let me know if you need anything else.
Flags: needinfo?(bob.silverberg)
(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.
Oops! Fixed.
LGTM, thanks Will.
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 months ago
Product: Toolkit → WebExtensions

Comment 41

5 months ago
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".
status-firefox57: wontfix → ---

Comment 42

5 months ago
Why is Bugzilla saying I changed "status-firefox57"? I didn't even see any options for changing any properties of that nature...
Thanks for the fix pikadudeno1!

Updated

2 months ago
Duplicate of this bug: 1488293
You need to log in before you can comment on or make changes to this bug.