1280121 - Re-enable the Font Panel

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Sebastian Zartner [:sebo]]

Bug 1247723 disabled the Font Panel by default with the justification that it is currently not very useful and it will be recreated at some point (see bug 1280059).

As several users complained about this, it should be re-enabled by default until there is a proper replacement of its features.

Sebastian

Comment 1

[Florian Rivoal]

This would be most simply accomplished by reverting the patch from Bug 1247723:

https://bugzilla.mozilla.org/attachment.cgi?id=8721429

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Florian Rivoal from comment #1)
> This would be most simply accomplished by reverting the patch from Bug
> 1247723:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8721429
Actually, just changing `false` to `true` on this line would work: http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js#326

Now, the font panel was disabled in 47. We got some complaints from a few users in nightly and aurora when it started to ride the train, but of course that's we have a small population of users on these branches. Now that it has hit release, we got more complaints since more people noticed.

If we fixed it now only, it would only be re-enabled in 50, unless we uplifted it, which we can probably pretty easily do up to aurora and beta, I'm unsure about release however.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug 1280121 - Re-enable the font panel by default

Review commit: https://reviewboard.mozilla.org/r/62042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62042/

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1280121 - Move the font panel to last position in the sidebar

Review commit: https://reviewboard.mozilla.org/r/62044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62044/

Comment 5

[Patrick Brosset <:pbro>]

I re-enabled the pref in a patch, and then took at stab at moving the font panel as first child of the tabbar, in a second patch.

Comment 6

[Patrick Brosset <:pbro>]

Ok, taking the final decision about the font panel: let's re-enable it now, and try to uplift this as much as we can.
There has been a couple of valid use cases exposed in bug 1247723, and although we have the right bugs on file to make the font-panel better (see bug 1280059), we're not planning to work on them right now.
Plus, bug 1247729 will move the box-model to the computed-view tab, which will free some space in the tabbar.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8767594 [details]
Bug 1280121 - Move the font panel to last position in the sidebar

https://reviewboard.mozilla.org/r/62044/#review58766

Code change looks good, thanks ! I would just like to discuss the public API for sidebar::addTab.

::: devtools/client/framework/sidebar.js:225
(Diff revision 1)
> -   * @param {string} url
> +   * @param {string} url The URL of the document to load in this new tab.
> +   * @param {Boolean} selected Set to true to make this new tab selected by default.
> +   * @param {Number} insertionIndex By default, the new tab is appended at the end of the
> +   * tabbox, use this index to insert it somewhere else.
>     */
> -  addTab: function (id, url, selected = false) {
> +  addTab: function (id, url, selected = false, insertionIndex = -1) {

I feel like the signature of this method starts to be cryptic. 

addTab("mypanel", "my-panel.xhtml", false, 3);

Two suggestions here:
 - what about regrouping the last two optional parameters as a unique object parameter?
 - using the tabid instead of an index ({insertBefore: "fontinspector"})

I think these two changes would make the client code easier to understand, but up to you!

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8767593 [details]
Bug 1280121 - Re-enable the font panel by default

https://reviewboard.mozilla.org/r/62042/#review58780

LGTM, thanks!

Comment 9

[Julian Descottes [:jdescottes]]

Another thing to consider if we move the font inspector as the last tab: we will need to uplift Bug 1267503 alongside this one in order to make sure the panel is accessible in the "all-tabs" menu.

I initially decided not to ask for the uplift of Bug 1267503, because the font menu was in 3rd position, so always visible anyway.

The other solution would be to only uplift part1 of this bug.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> The other solution would be to only uplift part1 of this bug.
Sounds good to me. I'll move the other patch to a different bug to make that easy.
Thanks for the reviews.

Comment 11

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6ad35a0b859&group_state=expanded

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/99b859906a9c
Re-enable the font panel by default

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8767593 [details]
Bug 1280121 - Re-enable the font panel by default

Pushed this change to fx-team.
It's just a pref default value change, and all tests on TRY still pass.
Of course, we still need to wait until it hits m-c, but I'm already asking for approval to uplift this to aurora and beta:

Approval Request Comment
[Feature/regressing bug #]: bug 1247723
[User impact if declined]: If declined, devtools users using the inspector will not be able to access the "font inspector" panel which lists all fonts used on the current page. They'll only be able to use it after they flip a pref in about:config. With this change, the font panel will be available by default.
It was enabled before, and we turned it OFF for various reasons, but decided to turn it ON by default again.
[Describe test coverage new/current, TreeHerder]: The whole font-inspector is pretty well tested already, no need for new tests for this patch alone.
[Risks and why]: There are no risks related to this change. We're only switching a pref to TRUE by default, and it used to be TRUE before 47 anyway.
[String/UUID change made/needed]: None

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/99b859906a9c

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8767593 [details]
Bug 1280121 - Re-enable the font panel by default

After discussing with :pbro, we got enough valid compliant about disabling the panel. So, we decided to re-enable it. Take it in 48 beta 6 and aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a72bfa56d50

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/673b03845fd8

Comment 18

[Sebastian Zartner [:sebo]]

Updated the note at https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/View_fonts and listed the change at https://developer.mozilla.org/en-US/Firefox/Releases/48.

Sebastian

Comment 19

[Tanvir Rahman]

I have reproduced this bug with Nightly 50.0a1(2016/06/14) on Windows 10, 64 bit!

The Bug's fix is now verified Latest Aurora 49.0a2 and Nightly 50.0a1.

Aurora 	49.0a2
Build ID 	20160727004019
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Nightly 50.0a1
Build ID 	20160727030230
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160727]