Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Description • 8 years ago

The font panel is not currently very useful. According to Patrick/telemetry, the average user spends ~5 seconds it in, and it seems like even that might be accidental: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-01-25&keys=__none__!__none__!__none__&max_channel_version=aurora%252F45&measure=DEVTOOLS_FONTINSPECTOR_TIME_ACTIVE_SECONDS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-12-17&table=0&trim=1&use_submission_date=0

I'd like to eventually create a font panel that's robust and useful (exposes Opentype features like ligatures and smallcaps, allows you to find glyphs in a font file, things of that nature). I'm proposing until we have the bandwidth to work on something of that nature we disable the font panel, as it creates additional, unuseful noise in what are fairly crowded devtools as it is.

According to Patrick this should be fairly easy and involves setting devtools.fontinspector.enabled to false.

	mark.kazakevich
	Comment 1 • 8 years ago

~University of Toronto, CSC302H1S-2016 assignment. Patch by Feb 26,2016~

I would like to work on this as my first FF bug fix.
I'm running on Linux Mint. I have checked out the gecko-dev source code, built FF 46 and am able to run it.

1) Would somebody advise me where to look for the necessary source code?
2) Any tips/suggestions? 

Thank you.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 2 • 8 years ago

(In reply to mark.kazakevich from comment #1)
> ~University of Toronto, CSC302H1S-2016 assignment. Patch by Feb 26,2016~
> 
> I would like to work on this as my first FF bug fix.
> I'm running on Linux Mint. I have checked out the gecko-dev source code,
> built FF 46 and am able to run it.
> 
> 1) Would somebody advise me where to look for the necessary source code?
> 2) Any tips/suggestions? 
> 
> Thank you.

Hey Mark! Thanks for volunteering!

Patrick Brosset is the mentor for this bug. On IRC, he's :pbro—there are some instructions on getting on IRC here: https://wiki.mozilla.org/IRC 

The channel you can ask questions in is #devtools.

The code for devtools is at the top level, /devtools/. You'll have to grep for the right flag.

I'm going to assign the bug to you on Bugzilla.

	Vishnu (:malayaleecoder) Assignee
	Comment 3 • 8 years ago

@mark: Please let me know if you are still going for the fix, else I am ready to take this up!

	Vishnu (:malayaleecoder) Assignee
	Comment 4 • 8 years ago

@Patrick: looks like mark is not active for almost a week. Can I have a go at this,if you don't mind ?

	mark.kazakevich
	Comment 5 • 8 years ago

Hey, sorry I've have been on break this week.
However, you may take it. I'm likely going with something else now.
Cheers.

	Vishnu (:malayaleecoder) Assignee
	Comment 6 • 8 years ago

Oh, Thanks Mark!

Patrick, should I change the boolean value for 'devtools.fontinspector.enabled' present in,
obj-x86_64-unknown-linux-gnu/dist/bin/browser/defaults/preferences/devtools.js 
also?

	Patrick Brosset <:pbro>
	Comment 7 • 8 years ago

(In reply to malayaleecoder from comment #6)
> Patrick, should I change the boolean value for
> 'devtools.fontinspector.enabled' present in,
> obj-x86_64-unknown-linux-gnu/dist/bin/browser/defaults/preferences/devtools.
> js 
> also?
No, the directory that starts with obj- is your build directory, it's created automatically by mach when you build firefox, and it contains the resulting binaries. You never need to change anything in that directory.

	Vishnu (:malayaleecoder) Assignee
	Comment 8 • 8 years ago

Hello Patrick,

I have done the changes as said in comment #1 in this patch.
But on running ./mach run, I still see the "Font & Colors" panel.
Is there something wrong that I have done, or am I addressing the wrong panel?

	Brian Grinstead [:bgrins]
	Comment 9 • 8 years ago

(In reply to malayaleecoder from comment #8)
> Created attachment 8720814 [details] [diff] [review]
> Bug1247723_v1.diff
> 
> Hello Patrick,
> 
> I have done the changes as said in comment #1 in this patch.
> But on running ./mach run, I still see the "Font & Colors" panel.
> Is there something wrong that I have done, or am I addressing the wrong
> panel?

Try running `./mach build faster` after making the changes but before doing `./mach run`

	Vishnu (:malayaleecoder) Assignee
	Comment 10 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to malayaleecoder from comment #8)
> > Created attachment 8720814 [details] [diff] [review]
> > Bug1247723_v1.diff
> > 
> > Hello Patrick,
> > 
> > I have done the changes as said in comment #1 in this patch.
> > But on running ./mach run, I still see the "Font & Colors" panel.
> > Is there something wrong that I have done, or am I addressing the wrong
> > panel?
> 
> Try running `./mach build faster` after making the changes but before doing
> `./mach run`

Brian, I did do the './mach build faster' before './mach run', but am still unable to find any changes!
Any help?

	Brian Grinstead [:bgrins]
	Comment 11 • 8 years ago

You mention the "Font & Colors" panel - I'm not sure what that is.  This is a sidebar panel inside the Inspector tool. See "Fonts View" here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/View_fonts

	Patrick Brosset <:pbro>
	Comment 12 • 8 years ago

I think that's because you have the pref defined in about:config. The fix for this bug is to just turn the default value of that pref to false. But for everyone that already has that pref set, the set value will still be taken into account.
Open about:config, find the pref, and delete it from there. Then re-start devtools

	Vishnu (:malayaleecoder) Assignee
	Comment 13 • 8 years ago

Thank You Brian, I was actually looking at the wrong panel!

@Patrick, I think the bug is fixed, I can no longer see the Font Panel on running. Please verify and tell me if further improvements are required.

Sorry for the confusion :\

	Patrick Brosset <:pbro>
	Comment 14 • 8 years ago

Comment on attachment 8720814 [details] [diff] [review]
Bug1247723_v1.diff

Review of attachment 8720814 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Now, we need to make sure the tests do work still. There are a bunch of tests for the font panel. I was going to suggest that we disable them, but the thing is, what we're doing here is simply turn off the pref, which means people can still turn it on. And for them, we want the panel to still work. We therefore want the tests to still run, especially to catch possible regressions in the future.
Tests are in \devtools\client\inspector\fonts\test\
So, to make sure the tests still work, you'll need to add something like this to the head.js file in this directory (this file is run before each and every test):

Services.prefs.setBoolPref("devtools.fontinspector.enabled", true);
registerCleanupFunction(() => {
  Services.prefs.clearUserPref("devtools.fontinspector.enabled");
});

This will turn the pref ON before the tests start, and will turn it back OFF when the tests end.
Once done, please do run the tests locally to make sure they work: ./mach test devtools/client/inspector/fonts/test/

Also, can you please change the commit message to be:
"Bug 1247723 - Disable the Font Panel; r=pbro"

	Vishnu (:malayaleecoder) Assignee
	Comment 15 • 8 years ago

Hey Patrick,

I have made the suggested changes and all the tests ran successfully!
Please have a look.

Thank You

	Patrick Brosset <:pbro>
	Comment 16 • 8 years ago

Comment on attachment 8721429 [details] [diff] [review]
Bug1247723_v2.diff

Review of attachment 8721429 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
I have pushed this patch to CI to make sure all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e8e0bf8246

	Patrick Brosset <:pbro>
	Comment 17 • 8 years ago

Alright, the TRY push is green, and the patch is R+, this is ready to land on fx-team.
Bye bye fonts panel, see you soon!

	Vishnu (:malayaleecoder) Assignee
	Comment 18 • 8 years ago

Thanks Patrick for helping me out :-)

	Lina Butler [:lina]
	Comment 19 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/b3175dd140e73e6d4875a99d4299a15539571f01
Bug 1247723 - Disable the Font Panel; r=pbro

	Lina Butler [:lina]
	Comment 20 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b3175dd140e7

	Will Bamberg [:wbamberg]
	Comment 21 • 8 years ago

doc updates:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/View_fonts#Fonts_view
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour#Fonts_view

	Gavin
	Comment 22 • 8 years ago

just wanted to give a bit of feedback as a user of the font panel, ~5 seconds is actually plenty for what it currently offers so that metric alone may not be the best data to use for deciding to disable it by default.

I for example, use it to ensure the correct font has loaded when using multiple fonts in a family. Using the CSS inspector alone isnt enough as it doesnt tell you which fonts are in use, just what was declared. A quick toggle of the font tab to dbl check is all that's needed.

The font panel is one my chrome using colleagues are a little envious of.

	Brian Grinstead [:bgrins]
	Comment 23 • 8 years ago

(In reply to Gavin from comment #22)
> just wanted to give a bit of feedback as a user of the font panel, ~5
> seconds is actually plenty for what it currently offers so that metric alone
> may not be the best data to use for deciding to disable it by default.
> 
> I for example, use it to ensure the correct font has loaded when using
> multiple fonts in a family. Using the CSS inspector alone isnt enough as it
> doesnt tell you which fonts are in use, just what was declared. A quick
> toggle of the font tab to dbl check is all that's needed.

Good point - that's use case that the rule view doesn't cover.  We have Bug 994559 on file which is about enhancing the rule view / computed view to somehow highlight only the applied font.  In fact, that would be more convenient than needing to switch to another panel.

	Jeff Muizelaar [:jrmuizel]
	Comment 24 • 8 years ago

It seems like disabling this is a little user hostile. I'm not sure what we gain from breaking existing users before we have a working replacement for exposing this information.

See this reddit thread for more:
https://www.reddit.com/r/firefox/comments/4itktq/starting_in_firefox_47_the_fonts_view_is_disabled/

	flying sheep
	Comment 25 • 8 years ago

Why would you remove it without a replacement ready?

This is just useful information being gone.

Please revert this.

	yisibl
	Comment 26 • 8 years ago

Update https://developer.mozilla.org/en-US/Firefox/Releases/47#Developer_Tools

	yisibl
	Comment 27 • 8 years ago

Regardless of the developer or designer, the font panel is very useful. I even suggested that Chrome add a separate font panel. I would like to be able to re design the layout of the panel instead of the default disable it.

	yisibl
	Comment 29 • 8 years ago

Comment on attachment 8761027 [details]
Chrome-DevTools.png

Chrome Devtools will automatically fold the panel.

	fireattack [:fireattack]
	Comment 30 • 8 years ago

Isn't font inspection tool *essential* for web developers, especially frond-end developers? Disabling it by default (without a replacement) will just push people to use Chrome to do web dev further more in my humble opinion.

And I didn't get the logic that "it's not currently very useful because people rarely use it". Usefulness and usage frequency / popularity are two different things: to make a rough analogy, maybe only 0.001% people on the Earth ever use a Geiger counter but no one will claim it's "not very useful". 

It's funny because you said you are going to add more features in the future such as "exposes Opentype features like ligatures and smallcaps, allows you to find glyphs in a font file, things of that nature": because these features will not increase a single bit of the popularity of fool inspector. Front-end dev will use it all the time, no matter if it sucks or not; while average Joe may never know it exists.

It's totally irrelevant to the features of the font tool. If you want to make it more "useful" and robust, Sure, why not? But if your reason to disable it is because "people don't use it enough", adding more features will not help this goal at all. Hence it's no need to disable it before the replacement is ready to ship.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 31 • 8 years ago

It is still possible to re-enable the font panel for your local use if you wish, it has only been hidden for now.

You can go to about:config and change devtools.fontinspector.enabled to true if you'd like to unhide it.

	Terrell Kelley
	Comment 32 • 8 years ago

Yes, it's been disabled. But you didn't put a preference in the actual DevTools settings, so people won't know that they can enable it.

	Julian Descottes [:jdescottes]
	Comment 33 • 8 years ago

Logged bug 1278984 in order to make the Fonts panel available from the sidebar menu.

	Florian Rivoal
	Comment 34 • 8 years ago

If I understand correctly, this is supposed to be replaced by https://bugzilla.mozilla.org/show_bug.cgi?id=994559. However, that seems short-sighted, since at least the initial description of that bug makes incorrect assumptions about how fonts work in CSS. It assumes that only one font in the list of fallback will be applied, which isn't true. At also glosses over the fact that one font family can be composed of several font faces, and white the Font View does give details about it, that suggestion wouldn't

I have nothing against replacing the Font View with some actually improved replacement, but retiring it in favor of a vague idea that has not yet been thought through let alone implemented, seems premature.

And yes, I know this is merely hidden, not removed; but this still effectively means the feature no longer exists for most people, and this is/was one of the areas where Firefox's dev tools are superior to the competition.

	Sebastian Zartner [:sebo]
	Comment 35 • 8 years ago

(In reply to Florian Rivoal from comment #34)
> If I understand correctly, this is supposed to be replaced by
> https://bugzilla.mozilla.org/show_bug.cgi?id=994559.

No, comment 0 says that the panel will be reworked at some point. The bug you link to covers only one feature of the Font Inspector. Having said that, I didn't see a bug filed for the new Font Inspector. Helen, is there already one?

> However, that seems
> short-sighted, since at least the initial description of that bug makes
> incorrect assumptions about how fonts work in CSS. It assumes that only one
> font in the list of fallback will be applied, which isn't true.

I don't see the initial description making such assumptions. It just says that the reworked Font Inspector should provide more features.

> I have nothing against replacing the Font View with some actually improved
> replacement, but retiring it in favor of a vague idea that has not yet been
> thought through let alone implemented, seems premature.
> 
> And yes, I know this is merely hidden, not removed; but this still
> effectively means the feature no longer exists for most people, and this
> is/was one of the areas where Firefox's dev tools are superior to the
> competition.

I absolutely agree on that.
The telemetry data doesn't answer important questions like how people use the tool or by how many people it is used intentionally. As Gavin mentioned in comment 22 five seconds may be enough for people to get from the panel what they want, so accidental switches to it can't be extracted from the telemetry data.

Sebastian

	Sebastian Zartner [:sebo]
	Comment 36 • 8 years ago

Btw. adding the panel back (or fixing bug 1278984) is not for the users carefully reading this bug or the docs and by that knowing that they can reenable the feature. It is for those users that suddenly see the Fonts panel gone for no obvious reason and not finding the option to get it back.

Sebastian

	Florian Rivoal
	Comment 37 • 8 years ago

>> However, that seems
>> short-sighted, since at least the initial description of that bug makes
>> incorrect assumptions about how fonts work in CSS. It assumes that only one
>> font in the list of fallback will be applied, which isn't true.
>
> I don't see the initial description making such assumptions. It just says that the reworked Font Inspector should provide more features.

By "that bug", I meant Bug 994559, which was the only one I could find listed from here as an alternative to this.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 38 • 8 years ago

Clearing my ni? based on comment #33, which will allow the Font panel to be re-enabled.

	Sebastian Zartner [:sebo]
	Comment 39 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)(limited avail 14-17) from comment #38)
> Clearing my ni? based on comment #33, which will allow the Font panel to be
> re-enabled.

I was asking whether there is already a bug filed for the *new* Font Inspector you mentioned in comment 0, not about reenabling the existing one.

Sebastian

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 40 • 8 years ago

No, no bug has been filed yet.

	Sebastian Zartner [:sebo]
	Comment 41 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)(limited avail 14-17) from comment #40)
> No, no bug has been filed yet.

Ok, so I've created bug 1280059 to track it. Feel free to add bugs blocking it.

Sebastian

	Paul Rouget [:paul]
	Comment 42 • 8 years ago

I'm surprised this happened.

Don't we want to fix the before-mentioned issues before removing this tool?

Also, I don't see how bug 994559 would help. The features offered by the font panel are not directly related to the rule view or css.

And what is bug 1280059 supposed to track?

	Sebastian Zartner [:sebo]
	Comment 43 • 8 years ago

(In reply to Paul Rouget [:paul] from comment #42)
> And what is bug 1280059 supposed to track?

The features Helen mentioned in comment 0 that are making it "robust and useful".

Sebastian

	Paul Rouget [:paul]
	Comment 44 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #43)
> (In reply to Paul Rouget [:paul] from comment #42)
> > And what is bug 1280059 supposed to track?
> 
> The features Helen mentioned in comment 0 that are making it "robust and
> useful".

Please expand. Lack of opentype features can't justify killing this tool, especially when it worked well.

As the author of this tool, I deserve some propers explanations on why this got killed without any proper replacements or a clear description of requirements for a v2.

	Paul Rouget [:paul]
	Comment 45 • 8 years ago

Patrick, can you please comment on this?

If we want to create a v2, why do we kill the v1 before the v2 is ready? What's the benefit?

	Sebastian Zartner [:sebo]
	Comment 46 • 8 years ago

(In reply to Paul Rouget [:paul] from comment #44)
> (In reply to Sebastian Zartner [:sebo] from comment #43)
> > (In reply to Paul Rouget [:paul] from comment #42)
> > > And what is bug 1280059 supposed to track?
> > 
> > The features Helen mentioned in comment 0 that are making it "robust and
> > useful".
> 
> Please expand. Lack of opentype features can't justify killing this tool,
> especially when it worked well.

Forwarding this request to Helen.
I agree with you that the current Fonts panel should be re-enabled independently of the work on new features. Therefore I have now created bug 1280121.

Sebastian

	Patrick Brosset <:pbro>
	Comment 47 • 8 years ago

One problem we often face with devtools is getting feedback. We can never know how, how much, and for what people use our tools. And so we try and take decisions as best we can to provide the best tools we can, in our opinions, given the feedback and data we have at our disposal.

The original intent behind hiding the fonts panel by default was that that it created additional noise in a fairly complex UI and, if you compared it to other panels in the inspector, didn't seem like the most important tool to have.

We want a less intimidating UI that is less crowded, more accessible to everyone with the tools people really need easily available. Again, with the data and feedback we had at our disposal, the fonts panel didn't appear to fit in that picture very well.

Also, one of the most important things we can do as team working on devtools is focusing our energy on less but better things. We used to have a very rapid expansion phase at the start where we didn't focus on anything but instead did millions of different things which, later, went unmaintained.

A consequence of wanting to focus more is that we might need to remove (or disable in this case) things we know we don't have the time to work on right now and properly finish.

So, we made a decision and went with it. And we learned here that people relied on that functionality more than we thought. I think people made really good points here.
So, thank you all for the feedback, and thank you Sebastian for taking care of filing and linking bugs.

I think we should stop the conversation here and direct our energy to the other bugs mentioned here, whether that means introducing a checkbox in the devtools settings panel, making the sidebar tabs widget look better/cleaner and still hold all our panels, working on a v2 now, or just straight on reverting the change.

	u525076
	Comment 48 • 8 years ago

Just updated to 47 and got surprised in 1 minute :D, came here by Googling "firefox inspector font removed".

In my opinion, at least for developers, cluttered UI is less worse than missing/hidden features. The font panel was very useful to me.

	Sebastian Zartner [:sebo]
	Comment 49 • 8 years ago

Bagana, please read comment 47, especially the last paragraph.
So, I advise you to follow the bugs listed under 'Blocks' and 'See Also', which are bug 1280121, bug 994559, bug 1278984 and bug 1280059.

Sebastian