Closed Bug 1400805 Opened 6 years ago Closed 6 years ago

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

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

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

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [browserSettings] triaged)

Attachments

(1 file, 1 obsolete file)

Bug 1400805 - added WebExtension API to get/set browser.display.use_document_fonts
59 bytes, text/x-review-board-request
zombie
: review+
Details 
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.
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)
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)
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)
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
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"
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
Thank you, Caitlin. I already had some time, so I did the changes and submitted a mozreview request.
Assignee: nobody → cadeyrn
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).
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'.
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 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 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.
> "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.
Attachment #8965237 - Attachment is obsolete: true
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 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 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+
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)
Flags: needinfo?(cadeyrn)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/10697d111e50
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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. :)
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)
Product: Toolkit → WebExtensions
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 → ---
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!
You need to log in before you can comment on or make changes to this bug.