Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: hholmes, Assigned: malayaleecoder, Mentored)

Tracking

({dev-doc-complete})

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Mentor: pbrosset
Component: Developer Tools: Inspector → Developer Tools: Font Inspector
Whiteboard: [good first bug]
~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.
(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.
Assignee: nobody → mark.kazakevich
@mark: Please let me know if you are still going for the fix, else I am ready to take this up!
Flags: needinfo?(mark.kazakevich)
@Patrick: looks like mark is not active for almost a week. Can I have a go at this,if you don't mind ?
Flags: needinfo?(pbrosset)
Hey, sorry I've have been on break this week.
However, you may take it. I'm likely going with something else now.
Cheers.
Flags: needinfo?(mark.kazakevich)
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?
(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.
Assignee: mark.kazakevich → malayaleecoder
Flags: needinfo?(pbrosset)
Posted patch Bug1247723_v1.diff (obsolete) — Splinter Review
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?
Flags: needinfo?(pbrosset)
(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`
(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?
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
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
Flags: needinfo?(bgrinstead)
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
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 :\
Flags: needinfo?(bgrinstead)
Flags: needinfo?(pbrosset)
Attachment #8720814 - Flags: review?(pbrosset)
Flags: needinfo?(bgrinstead)
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"
Attachment #8720814 - Flags: review?(pbrosset)
Hey Patrick,

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

Thank You
Attachment #8720814 - Attachment is obsolete: true
Attachment #8721429 - Flags: review?(pbrosset)
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
Attachment #8721429 - Flags: review?(pbrosset) → review+
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!
Keywords: checkin-needed
Thanks Patrick for helping me out :-)
https://hg.mozilla.org/mozilla-central/rev/b3175dd140e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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.
(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.
See Also: → 994559
Depends on: 1259892
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/
Why would you remove it without a replacement ready?

This is just useful information being gone.

Please revert this.
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.
Posted image Chrome-DevTools.png
Comment on attachment 8761027 [details]
Chrome-DevTools.png

Chrome Devtools will automatically fold the panel.
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.
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.
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.
Logged bug 1278984 in order to make the Fonts panel available from the sidebar menu.
See Also: → 1278984
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.
(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
Flags: needinfo?(hholmes)
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
>> 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.
Clearing my ni? based on comment #33, which will allow the Font panel to be re-enabled.
Flags: needinfo?(hholmes)
(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
Flags: needinfo?(hholmes)
No, no bug has been filed yet.
Flags: needinfo?(hholmes)
See Also: → 1280059
(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
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?
(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
(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.
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?
Flags: needinfo?(pbrosset)
(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
Flags: needinfo?(hholmes)
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.
Flags: needinfo?(pbrosset)
Flags: needinfo?(hholmes)
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.
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.