Closed Bug 1259892 Opened 4 years ago Closed 4 years ago

Empty Font Panel is accessible via overflow dropmarker in sidebar

Categories

(DevTools :: Inspector: Fonts, defect, P1)

defect

Tracking

(firefox45 unaffected, firefox46 unaffected, firefox47 affected, firefox48 fixed, firefox-esr38 unaffected, firefox-esr45 unaffected)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- affected
firefox48 --- fixed
firefox-esr38 --- unaffected
firefox-esr45 --- unaffected

People

(Reporter: arni2033, Assigned: AMoz, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js][btpp-fix-now])

Attachments

(1 file, 5 obsolete files)

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160324030447
STR:
1. Open Inspector, select <body>, resize ruleview so that it was as narrow as possible
2. Click overflow dropmarker
3. Click "Fonts"

AR:  Fonts panel opens, but it's completely empty and looks broken.
ER:  There should be no way to open Fonts panel.
The fonts panel was taken down a few weeks ago in bug 1247723. 
The way this was done is by simply turning pref devtools.fontinspector.enabled to false by default.
However, it looks like that's not enough to hide it from the "all tabs" menu in the sidebar panel.
In fact, the fonts tab is still present in the DOM (see \devtools\client\inspector\inspector.xul, element #sidebar-tab-fontinspector), but only hidden (see \devtools\client\inspector\inspector-panel.js, in the setupSidebar function).

In this bug, we should make a change to the sidebar widget so it doesn't add hidden tabs to the "all tabs" menu.
This can be done in \devtools\client\framework\sidebar.js in the addAllTabsMenu function.

Marking this as a P1 because it's quite embarrassing, but it's also a pretty easy one to fix if anyone is interested in getting it fixed.

Please make sure you have the dev environment set up and know how to make changes to devtools and reload those changes first: https://wiki.mozilla.org/DevTools/Hacking
Mentor: pbrosset
Priority: -- → P1
Whiteboard: [good first bug][lang=js][btpp-fix-now]
Hello,
I am returning to Bugzilla after a really long time. Hence thought of picking this bug as a fresh start.
(In reply to Amod [:AMoz] from comment #2)
> Hello,
> I am returning to Bugzilla after a really long time. Hence thought of
> picking this bug as a fresh start.
Thanks Amod, that'd be great.
Not sure if you've contributed to devtools in the past. If not, please go through our contribution guide on MDN:
https://developer.mozilla.org/en-US/docs/Tools/Contributing
I'll assign the bug to you now. Feel free to ping me here or on IRC (#devtools) for more info if needed.
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
From what I could understood from the code. I tried to search which is the attribute to detect whether the tab is hidden or not. I figured that "tab.hidden" is the attribute.

I tried to build the system which failed. Any guidance on this patch would be helpful for me.
Attachment #8737257 - Flags: feedback?(pbrosset)
Actually, if user selected Font Panel before updating to 47, the empty Fonts Panel will be opened the next time user opens Inspector.  Is this bug going to fix this case?
(In reply to arni2033 from comment #5)
> Actually, if user selected Font Panel before updating to 47, the empty Fonts
> Panel will be opened the next time user opens Inspector.  Is this bug going
> to fix this case?
The patch submitted in comment 4 is not going to fix this. But I agree that it should.
This should be an easy fix in devtools/client/inspector/inspector-panel.js, in the setupSidebar function:
If the pref devtools.fontinspector.enabled is false *and* if the defaultTab is "fontinspector", then we should default to "ruleview" instead.

Amod: can you add this fix too? I will now review your patch.
Flags: needinfo?(amod.narvekar)
Comment on attachment 8737257 [details] [diff] [review]
Added a condition to check whether tab is hidden or not.

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

This is the right fix. Thanks!
See comment 6 for an additional fix to do too.
Can you please give more information about what failed with you tried to build? Once you've build fully once with './mach build', after you make JS/CSS changes, you can simply build with './mach build faster', this should take a few seconds, and you can then test your changes with './mach run -P dev'.

::: devtools/client/framework/sidebar.js
@@ +146,5 @@
>  
>      // Add menuitems to the alltabs menu if there are already tabs in the
>      // sidebar
>      for (let [id, tab] of this._tabs) {
> +      if(!tab.hidden){ 

The formatting here is not consistent with the devtools coding guidelines. This is really a minor comment, but it helps to have a consistent coding style, it makes reading the code easier. Find out more information about this here: https://wiki.mozilla.org/DevTools/CodingStandards

For this particular case, it should be:

if (!tab.hidden) {
Attachment #8737257 - Flags: feedback?(pbrosset) → feedback+
Attached patch Defaut Tab assignment (obsolete) — Splinter Review
I doubt the assignment statement syntax which I have given. Regarding the placement of the statement, I have placed them after the "defaultTab" is declared and before the show() function. Your review would be helpful to advance it.

Regarding tests, since I have hosed up mercurial, there are plenty of reject messages, So I would set up the system again.
Attachment #8737257 - Attachment is obsolete: true
Flags: needinfo?(amod.narvekar)
Attachment #8738643 - Flags: review?(pbrosset)
Comment on attachment 8738643 [details] [diff] [review]
Defaut Tab assignment

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

I few more changes are needed, but the fix is essentially here.

::: devtools/client/framework/sidebar.js
@@ +146,5 @@
>  
>      // Add menuitems to the alltabs menu if there are already tabs in the
>      // sidebar
>      for (let [id, tab] of this._tabs) {
> +      if(!tab.hidden) {

nit: formatting is inconsistent with our coding rules, see comment 7.

::: devtools/client/inspector/inspector-panel.js
@@ +370,5 @@
>                            "chrome://devtools/content/animationinspector/animation-inspector.xhtml",
>                            "animationinspector" == defaultTab);
>      }
>  
> +    if(!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&

nit: formatting is inconsistent with our coding rules, see comment 7.

@@ +371,5 @@
>                            "animationinspector" == defaultTab);
>      }
>  
> +    if(!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> +       defaultTab == this.fontInspector) {

this.fontInspector is not a string, it's the instance of the fontInspector object.
And on the line below, this.ruleview isn't a string either, it's an object.
However, defaultTab should be a string. It's supposed to the the name of the tab, not the object of the panel inside the tab.

So this should be:

if (!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
    defaultTab == "fontinspector") {
  defaultTab = "ruleview";
}

Also, it would make more sense to move this 'if' block right after defaultTab is initialized. So, a few lines above, right after this:
let defaultTab = Services.prefs.getCharPref("devtools.inspector.activeSidebar");

@@ +372,5 @@
>      }
>  
> +    if(!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> +       defaultTab == this.fontInspector) {
> +      defaultTab=this.ruleview;

Formatting is not correct here too, please add a space before and after the = sign.
Attachment #8738643 - Flags: review?(pbrosset)
Attached patch bug-1259892-fix (obsolete) — Splinter Review
Performed the needful operations. Apologies for the deprivation of the syntax conventions.
Attachment #8738643 - Attachment is obsolete: true
Attachment #8738920 - Flags: review?(pbrosset)
I built the entire system. It was successful without any error.
Comment on attachment 8738920 [details] [diff] [review]
bug-1259892-fix

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

One final change is needed before we can land this.

::: devtools/client/inspector/inspector-panel.js
@@ +348,5 @@
>  
>      let defaultTab = Services.prefs.getCharPref("devtools.inspector.activeSidebar");
>  
> +    if (!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> +       defaultTab == "fontInspector") {

The id of the font inspector tab is "fontinspector", not "fontInspector".
Strings are being compared in a case-sensitive manner, so using "fontInspector" won't work as you'd expect.
Attachment #8738920 - Flags: review?(pbrosset)
Sorry once again.
Attachment #8738920 - Attachment is obsolete: true
Attachment #8740357 - Flags: review?(pbrosset)
i had accidentally uploaded the same patch again. Here is the modified one.
Attachment #8740357 - Attachment is obsolete: true
Attachment #8740357 - Flags: review?(pbrosset)
Attachment #8740358 - Flags: review?(pbrosset)
Comment on attachment 8740358 [details] [diff] [review]
Replaced "fontInspector" to "fontinspector"

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

Looks good now, thanks!
Attachment #8740358 - Flags: review?(pbrosset) → review+
Hi Amod! Looks like this fix is almost ready to land. It's marked as P1, so let's try to move forward :) 

I pushed your current patch to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb51d305afb .
If the tests on try are successful we should be ready to land this.

Your commit message should end with "r=pbro" instead of "r:pbro". Otherwise the reviewer will not be picked up by automated tools. Could you amend the commit message and re-upload your patch ?

Thanks!
Flags: needinfo?(amod.narvekar)
Uploaded with the required modification. I went though your try-server links, but unable to understand anything of it.
Attachment #8740358 - Attachment is obsolete: true
Flags: needinfo?(amod.narvekar) → needinfo?(jdescottes)
(In reply to Amod [:AMoz] from comment #17)
> Created attachment 8744377 [details] [diff] [review]
> modified to r=pbro
> 
> Uploaded with the required modification. I went though your try-server
> links, but unable to understand anything of it.

Thanks for the updating the patch so quickly! And sorry for dropping the try link without any explanation :)

Before landing a patch, we usually submit it to our continuous integration server (called try).
The devtools test suites will run on various platforms to check that the patch didn't break anything we could have missed.
When a test suite comes out orange, it means a test failed in this suite. We are not necessarily after a 100% success rate, we just want to check that no test failure comes from the new patch.

We have one unknown test failure here, but it's not related to your change. The others are known intermittents. I think we are good to go here. 

I am adding the "checkin-needed" keyword to this bug. One of the sheriffs will then pickup your patch and push it to the fx-team branch. If it sticks, it will then move to the mozilla-central branch and will make its way to the next release. We might make it in time for FF48.

Thanks a lot for fixing this Amod!
Flags: needinfo?(jdescottes)
Keywords: checkin-needed
Thank you very much for the help and support. Looking forward for more such bugs. Let me know in case of any in the same domain.
https://hg.mozilla.org/mozilla-central/rev/e976a219a56c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Summary: Empty Font Panel is accessible via overflow dropmrker in sidebar → Empty Font Panel is accessible via overflow dropmarker in sidebar
Depends on: 1267503
Duplicate of this bug: 1280221
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.