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)
WebExtensions
General
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
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)
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•6 years ago
|
||
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•6 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P5
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
> 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•6 years ago
|
||
Bob, we should ensure then we have matching (and probably blocking bug) for the about:preferences page as well.
Comment 5•6 years ago
|
||
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 7•6 years ago
|
||
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.
Updated•6 years ago
|
Whiteboard: [browserSettings] triaged
Comment hidden (advocacy) |
Comment 9•6 years ago
|
||
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•6 years ago
|
||
I would like to try it on the weekend as my first contribution to Firefox.
Comment 11•6 years ago
|
||
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•6 years ago
|
||
Thank you, Caitlin. I already had some time, so I did the changes and submitted a mozreview request.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → cadeyrn
Comment 14•6 years 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•6 years 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•6 years ago
|
Attachment #8965237 -
Flags: review?(ntim.bugs)
Comment 17•6 years ago
|
||
Thanks Sören, please flag me for final review once you get r+ from :ntim.
Mentor: tomica → ntim.bugs
Comment 18•6 years 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•6 years 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)
Comment 20•6 years ago
|
||
(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•6 years 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!
Comment 22•6 years ago
|
||
> 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•6 years ago
|
Attachment #8965237 -
Attachment is obsolete: true
Comment 24•6 years 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+
Comment 25•6 years ago
|
||
(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•6 years 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)
Comment 27•6 years ago
|
||
> the ode looks good,
And the code is well written too.
Comment hidden (mozreview-request) |
Comment 29•6 years 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•6 years 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).
Comment 32•6 years ago
|
||
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•6 years ago
|
Flags: needinfo?(cadeyrn)
Keywords: checkin-needed
Comment 33•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10697d111e50
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 35•6 years ago
|
||
\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•6 years 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)
Updated•6 years ago
|
Flags: needinfo?(cadeyrn) → qe-verify-
Comment 37•6 years ago
|
||
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)
Comment 38•6 years ago
|
||
(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•6 years ago
|
||
Oops! Fixed.
Updated•6 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 41•6 years 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•6 years ago
|
||
Why is Bugzilla saying I changed "status-firefox57"? I didn't even see any options for changing any properties of that nature...
Comment 43•6 years ago
|
||
Thanks for the fix pikadudeno1!
You need to log in
before you can comment on or make changes to this bug.
Description
•