Closed Bug 1280121 Opened 8 years ago Closed 8 years ago

Re-enable the Font Panel

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox48 verified, firefox49 verified, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: sebo, Assigned: pbro)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [bugday-20160727])

Attachments

(1 file, 1 obsolete file)

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
This would be most simply accomplished by reverting the patch from Bug 1247723:

https://bugzilla.mozilla.org/attachment.cgi?id=8721429
(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.
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.
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.
Priority: -- → P1
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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!
Attachment #8767594 - Flags: review?(jdescottes)
Comment on attachment 8767593 [details]
Bug 1280121 - Re-enable the font panel by default

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

LGTM, thanks!
Attachment #8767593 - Flags: review?(jdescottes) → review+
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.
(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.
Attachment #8767594 - Attachment is obsolete: true
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/99b859906a9c
Re-enable the font panel by default
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
Attachment #8767593 - Flags: approval-mozilla-beta?
Attachment #8767593 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/99b859906a9c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Keywords: dev-doc-needed
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.
Attachment #8767593 - Flags: approval-mozilla-beta?
Attachment #8767593 - Flags: approval-mozilla-beta+
Attachment #8767593 - Flags: approval-mozilla-aurora?
Attachment #8767593 - Flags: approval-mozilla-aurora+
Whiteboard: [bugday-20160727]
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]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: