Re-enable the Font Panel

VERIFIED FIXED in Firefox 48

Status

P1
normal
VERIFIED FIXED
3 years ago
8 months ago

People

(Reporter: sebo, Assigned: pbro)

Tracking

({dev-doc-complete})

Trunk
Firefox 50
dev-doc-complete

Firefox Tracking Flags

(firefox48 verified, firefox49 verified, firefox50 verified)

Details

(Whiteboard: [bugday-20160727])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

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

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

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

3 years ago
Created attachment 8767593 [details]
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/
Attachment #8767593 - Flags: review?(jdescottes)
Attachment #8767594 - Flags: review?(jdescottes)
(Assignee)

Comment 4

3 years ago
Created attachment 8767594 [details]
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/
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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)

Updated

3 years ago
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.
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8767594 - Attachment is obsolete: true

Comment 12

3 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/99b859906a9c
Re-enable the font panel by default
(Assignee)

Comment 13

3 years ago
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?
status-firefox48: --- → affected
status-firefox49: --- → affected

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99b859906a9c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Reporter)

Updated

3 years ago
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+

Comment 16

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a72bfa56d50
status-firefox49: affected → fixed

Comment 17

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/673b03845fd8
status-firefox48: affected → fixed
(Reporter)

Comment 18

3 years ago
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
Keywords: dev-doc-needed → dev-doc-complete
status-firefox48: fixed → verified
Whiteboard: [bugday-20160727]

Comment 19

3 years ago
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]

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.