Closed Bug 1267503 Opened 5 years ago Closed 5 years ago

The font inspector does not display in devtools-sidebar-alltabs even if "devtools.fontinspector.enabled = true"

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 affected, firefox49 affected, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- affected
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: magicp.jp, Assigned: AMoz, Mentored)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 files, 4 obsolete files)

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

Steps to reproduce:

1. Start latest Nightly
2. Set devtools.fontinspector.enabled = true in about:config
3. Open DevTools > Inspector
4. Narrow inspector-sidebar until displaying devtools-sidebar-alltabs 
5. Open devtools-sidebar-alltabs


Actual results:

The font inspector does not display in devtools-sidebar-alltabs even if "devtools.fontinspector.enabled = true"


Expected results:

The font inspector displays in devtools-sidebar-alltabs when "devtools.fontinspector.enabled = true"
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Inspector bug triage. Filter on CLIMBING SHOES.

Not a blocking issue, but marking as P2 because it is a recent regression.
Component: Developer Tools: Inspector → Developer Tools: Font Inspector
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Had a quick look, I think this should be easy enough to fix. Dropping some pointers below if anyone wants to take it.

In sidebar.js::addAllTabsMenu, when a tab is hidden, the button should still be added to the allTabs menu but with a hidden property.
If you look at sidebar.js::toggleTab, if a hidden tab becomes visible, we try to flip the corresponding entry in the menu :
> this._allTabsBtn.querySelector("#sidebar-alltabs-item-" + id).hidden = !isVisible;

On top of that, in inspector-panel.js::setupSidebar, instead of setting hidden=true on the sidebar tab, we should go through the sidebar API and use this.sidebar.toggleTab in order to show the "fontinspector" tab.

Adding a sidebar test to verify this would be nice (devtools/client/framework/test).

Amod: You said you were looking for more bugs :) Would you like to take care of this follow up?
Mentor: jdescottes
Flags: needinfo?(amod.narvekar)
hi Julian,
Thank you very much for suggesting the bug. I would definitely like to work on it. I would upload the patch within a while.
Flags: needinfo?(amod.narvekar)
Great, thanks Amod, assigning the bug to you! 

Don't know if you had the chance to write mochitests previously, here is some documentation on how to run/write tests:
- https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
- https://wiki.mozilla.org/DevTools/mochitests_coding_standards

The easiest is often to look for similar tests that we could reuse. 
Look at devtools/client/framework/test/browser_toolbox_sidebar_overflow_menu.js to see how to test the menu.
This test adds tabs dynamically to the sidebar. For our use case, it would be good to use tabs defined in the markup (with one visible and one hidden for instance).
Look at devtools/client/framework/test/browser_toolbox_sidebar_existing_tabs.js to see how to setup a test with tabs defined in the markup.
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
Attached patch patchv1 (obsolete) — Splinter Review
I wasn't able to understand the changes in | inspector-panel.js |, hence I did them as per my understanding from the comment. I built (./mach build) the entire system and it was successful. Now proceeding towards tests. Meanwhile, if you can give a feedback on the patch, that would be great.
Attachment #8745393 - Flags: feedback?(jdescottes)
Comment on attachment 8745393 [details] [diff] [review]
patchv1

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

Thanks for submitting a first patch Amod, please have a look at my comments below!

::: devtools/client/framework/sidebar.js
@@ +151,5 @@
>          this._addItemToAllTabsMenu(id, tab, tab.hasAttribute("selected"));
>        }
> +      else {
> +        this._addItemToAllTabsMenu(id, tab, tab.hasAttribute("hidden"));        
> +      }

The 3rd argument to _addItemToAllTabsMenu is used to select one of the menu items.

Here we want to:
- add all tabs to the menu, hidden or not
- if the tab is hidden, we want to hide the created menu item as well. The menu item is the return value of "_addItemToAllTabsMenu" so we can use this here

So something like :
> for (let [id, tab] of this._tabs) {
>   let item = this._addItemToAllTabsMenu(id, tab, tab.hasAttribute("selected"));
>   if (tab.hidden) { 
>     item.hidden = true;
>   }
> }
>

Also it looks like you have some extra whitespace at the end of the line you added. We use eslint to verify our JS syntax -> have a look at https://wiki.mozilla.org/DevTools/CodingStandards

::: devtools/client/inspector/inspector-panel.js
@@ +364,5 @@
>  
>      if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
>          this.canGetUsedFontFaces) {
>        this.fontInspector = new FontInspector(this, this.panelWin);
> +      this.panelDoc.getElementById("sidebar-tab-fontinspector").setupSidebarToggle();

Here the goal is to use the sidebar API to remove the "hidden" attribute on the sidebar tab.

Something like :

> this.sidebar.toggleTab(true, "fontinspector");

The first argument to toggleTab is "isVisible", we pass true to "show" the tab.
The second argument is the tabId, here "fontinspector".
Attachment #8745393 - Flags: feedback?(jdescottes)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0

For me it's reversed in current beta6: font inspector tab is missing from the sidebar, when I shrink the sidebar until the dropdown shows, I can select Fonts from it but it only switches to a non-functional, empty panel in the sidebar.
Same issue?
(In reply to Simon from comment #8)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101
> Firefox/47.0
> 
> For me it's reversed in current beta6: font inspector tab is missing from
> the sidebar, when I shrink the sidebar until the dropdown shows, I can
> select Fonts from it but it only switches to a non-functional, empty panel
> in the sidebar.
> Same issue?

What you describe is actually Bug 1259892, which was fixed in Firefox 48 (current aurora/dev-edition).
If you need to use the font inspector you first need to set the preference devtools.fontinspector.enabled to true.
Amod: Are you still working on this Bug? Let me know if I can help. Also I realize this is probably not a good next bug, so it's totally fine if you would like to swap this bug for another one!
Flags: needinfo?(amod.narvekar)
Hi Julian,
Sorry I forgot to inform you that I was busy last week. I will continue to work on this work. I will upload the patch within a day.
Flags: needinfo?(amod.narvekar)
Attached patch patchv2 (obsolete) — Splinter Review
made the changes. Now looking forward to tests. If there are any syntactical errors like whitespace, then I request you to wait till I successfully install eslint as I am facing some problem with it. 

I am going through the sample test you have suggested in the earlier comment.
Attachment #8745393 - Attachment is obsolete: true
Comment on attachment 8755915 [details] [diff] [review]
patchv2

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

Thanks for the patch Amod, looks good!

Let me know if you need help with the eslint setup or the test creation.

For eslint, you should be able to set it up by running "./mach eslint --setup". 
Then you can execute it on the files for this patch by doing 

> ./mach eslint --no-ignore devtools/client/inspector/inspector-panel.js
> ./mach eslint --no-ignore devtools/client/framework/sidebar.js

::: devtools/client/inspector/inspector-panel.js
@@ +364,5 @@
>  
>      if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
>          this.canGetUsedFontFaces) {
>        this.fontInspector = new FontInspector(this, this.panelWin);
>        this.panelDoc.getElementById("sidebar-tab-fontinspector").hidden = false;

We no longer need to set hidden = false here, it will be taken care of by the line you added just below.

So you can remove the line:
this.panelDoc.getElementById("sidebar-tab-fontinspector").hidden = false;
Attachment #8755915 - Flags: feedback+
hi Julian,
I am unable to get to a conclusion after looking at the two test files you had mentioned. I need some more help regarding it.
Not sure how to explain it easily so here is an example of how to start this test. It should pass with your current patch, but it needs probably some cleanup.

It would be nice also to enhance it a bit. Right now it is just testing that the menu is updated when  showing an item. We should check that it is also updated when hiding an item. You can try to improve it along these lines if you want!

Let me know if you need more information about this!
Flags: needinfo?(amod.narvekar)
Hi Amod! 

I think we really need to have this bug integrated in Firefox 49. The merge date was last Monday, so this means we will have to ask for an uplift to aurora. The sooner we land this, the easier it will be. 

Can you update your patch, addressing my comment 13 (remove the line this.panelDoc.getElementById("sidebar-tab-fontinspector").hidden = false; in inspector-panel.js). I will add the test in a separate patch and take care of the uplift request.

Thanks!
Flags: needinfo?(amod.narvekar)
see previous comment
Flags: needinfo?(amod.narvekar)
Attached patch patchv3 (obsolete) — Splinter Review
Sorry for taking so long to submit this patch.
I have got a fair idea on seeing your test files.
Attachment #8745141 - Attachment is obsolete: true
Attachment #8755915 - Attachment is obsolete: true
Flags: needinfo?(amod.narvekar)
Attachment #8761504 - Flags: review?(jdescottes)
Comment on attachment 8761504 [details] [diff] [review]
patchv3

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

Thanks for the fast reply! Code change looks good.

Two small issues we need to address before landing:

- I couldn't apply your patch directly, can you rebase [1] your patch after updating your repository?
  Let me know if you need help with this.
  [1] https://www.mercurial-scm.org/wiki/RebaseExtension
- The commit message should mention the font inspector (e.g "Bug 1267503 - Show the font-inspector in sidebar menu if enabled, r=jdescottes" or something like that)
Attachment #8761504 - Flags: review?(jdescottes) → review+
Attached patch patchv4Splinter Review
The rebase didnt work properly. I got .rej file. Hence I made changes again. Will this patch work ?
Attachment #8761504 - Attachment is obsolete: true
Attachment #8762697 - Flags: review?(jdescottes)
Comment on attachment 8762697 [details] [diff] [review]
patchv4

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

Patch looks good and it applies cleanly, thanks a lot Amod! 

I just pushed your fix to try, in order to check that we are not breaking any existing feature:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc968ad1b422

If try is green we will ask for your patch to be checked in!
Attachment #8762697 - Flags: review?(jdescottes) → review+
Try is green, except for unrelated intermittents.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f2c47201a369
Show the font-inspector in sidebar menu if enabled, r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2c47201a369
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.