Closed
Bug 1267503
Opened 8 years ago
Closed 8 years ago
The font inspector does not display in devtools-sidebar-alltabs even if "devtools.fontinspector.enabled = true"
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(firefox48 affected, firefox49 affected, firefox50 fixed)
RESOLVED
FIXED
Firefox 50
People
(Reporter: magicp.jp, Assigned: AMoz, Mentored)
References
Details
(Whiteboard: [btpp-fix-later])
Attachments
(2 files, 4 obsolete files)
5.71 KB,
patch
|
Details | Diff | Splinter Review | |
2.16 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
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
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
status-firefox48:
affected → ---
Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=1aabbe445e730853e83e9585d888fe548320226a&tochange=e976a219a56c556433590cb67c8b238015da610d
Blocks: 1259892
status-firefox48:
--- → affected
Comment 2•8 years ago
|
||
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]
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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?
Comment 9•8 years ago
|
||
(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.
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2c47201a369
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
Component: Inspector: Fonts → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•