Closed Bug 1172412 Opened 6 years ago Closed 5 years ago

The Filters menu places the checkbox behind the markers

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

All
Linux
defect

Tracking

(firefox40 affected, firefox41 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox40 --- affected
firefox41 --- affected
firefox50 --- verified

People

(Reporter: avaida, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [good first bug][lang=js])

Attachments

(4 files, 2 obsolete files)

Reproducible on:
* Aurora 40.0a2 (2015-06-04)
* Nightly 41.0a1 (2015-06-07)

Affected platforms:
Linux-only, tested with Ubuntu 14.04 (x64)

Steps to reproduce:
1. Launch Firefox.
2. Toggle the Developer Tools and select "Performance".
3. Select "Waterfall" and then click the "Filters" button next to it.

Expected result:
The filters menu is displayed properly.

Actual result:
On Ubuntu 14.04, the checkboxes available for each filter is placed behind the color of each marker.

Notes:
* This issue might be a regression, I'll follow up with a regression range as soon as possible.
Wonder if related to bug 1172737
See Also: → 1172737
Likely fixed by 1172282
Depends on: 1172282
Oh wait, this is a Linux bug, so this isn't fixed by that bug. But could be a regression from the same issue, bug 1047713.
No longer depends on: 1172282
Regression range:

mozilla-central
===============
Last good revision: af68c9c0e903
First bad revision: 436686833af0
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=af68c9c0e903&tochange=436686833af0

mozilla-inbound
===============
Last good revision: ada27d1eac50
First bad revision: 3ef1e4e8a61e
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ada27d1eac50&tochange=3ef1e4e8a61e
Whiteboard: [good first bug][lang=js]
Hey all,

I was able to reproduce this in my Linux dev environment. Would it be OK for me (JS noob) to work on this as a first bug?

Thank you!
Hey Jonathan, sorry for the delay -- are you still interested in this bug?
Flags: needinfo?(jonathan.wilfredo.fuentes)
Hey Jordan, yeah I'm still interested. Although I'm not entirely sure where to start.
Flags: needinfo?(jonathan.wilfredo.fuentes)
I don't have a linux machine to test, but I think a good place to start is the markup file that has this: browser/devtools/performance/performance.xul

In #performance-filter-popup is where this filter menu lives, and is constructed dynamically in browser/devtools/performance/views/toolbar.js within the _buildMarkersFilterPopup. The styling for these components can be found in browser/themes/shared/devtools/performance.css. All this, and using the browser toolbox[0] for debugging styles should be a good start. Let me know if you have any other questions!

[0] https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
Assignee: nobody → jonathan.wilfredo.fuentes
Flags: needinfo?(jsantell)
Thanks Jordan! I've started to take a look at the files you mentioned, but I couldn't quite figure it out yet. I'm going to carve out some time tomorrow morning to take another look.
Hey Jonathan, any updates? Anything I can help with?
Flags: needinfo?(jonathan.wilfredo.fuentes)
Hey Jordan,

Please excuse my lack of updates. I'll be sure to post more often in the future.

Anyways, I'm still having trouble getting those check marks to move. I tried grep'ing for 'checkbox' in the mozilla-central/browser/devtools/performance/ directory. I only got hits in performance.xul & toolar.js, so I focused on those files (similar grep searches for "check box" and "check-box" yielded no results):


1) I checked the order of attributes in performance.xul to see if this would impact the appearance. 
 	     <menuitem type="checkbox"
                       id="option-show-platform-data"

                	  vs

             <menuitem id="option-show-platform-data"
             	       type="checkbox"
                
   Unfortunately, this doesn't appear to have much effect.



2) I tried changing the order of building menuItem in toolbar.js (move menuitem.setAttribute("type", "checkbox") further down),but this doesn't appear to have any effect either.


I'm also unsure if I'm correctly building Firefox when I'm testing my changes. Whenever I've made changes to test out, I've been running "./mach build" and then "./mach run". Is this correct? According to https://wiki.mozilla.org/Good_first_bug#Update_to_the_latest_build, I should run "./mach build browser/".

Any tips would be much appreciated :)
Flags: needinfo?(jonathan.wilfredo.fuentes)
Hey Jordan,

I've continued to look at this, but I'm still having trouble. For instance, I've looked at #buildMarkersFilterPopup, and I've noticed that it seems to be iterating over a collection of menu items in an object called TIMELINE_BLUEPRINT. I found that this object is exported in browser/devtools/performance/modules/markers.js. Unfortunately, this file doesn't seem to have information regarding the order in which the menu items are built (e.g. checkmark rendered underneath checkbox). 

Looking back at performance.xul, I thought it would be a good idea to look at the XUL documentation for any clues. I've looked at the checkbox & menupopup to see if I could find anything similar to the z-index in CSS (which I thought could be relevant to fixing this). I did notice that there was one self-closing <menupopup> before the <menupopup> with all the checkmarks. I thought this might be somehow screwing things up, but removing that effectively disabled the performance checkbox menu altogether. 

I'm still going to try & fix this, but if you have any tips, I really appreciate it.
Flags: needinfo?(jsantell)
(In reply to Jonathan [:elefont] from comment #13)
> Hey Jordan,
> 
> I've continued to look at this, but I'm still having trouble. For instance,
> I've looked at #buildMarkersFilterPopup, and I've noticed that it seems to
> be iterating over a collection of menu items in an object called
> TIMELINE_BLUEPRINT. I found that this object is exported in
> browser/devtools/performance/modules/markers.js. Unfortunately, this file
> doesn't seem to have information regarding the order in which the menu items
> are built (e.g. checkmark rendered underneath checkbox). 

The order is based on the ordering of the keys in that object, as they're iterated via Object.keys


> Looking back at performance.xul, I thought it would be a good idea to look
> at the XUL documentation for any clues. I've looked at the checkbox &
> menupopup to see if I could find anything similar to the z-index in CSS
> (which I thought could be relevant to fixing this). I did notice that there
> was one self-closing <menupopup> before the <menupopup> with all the
> checkmarks. I thought this might be somehow screwing things up, but removing
> that effectively disabled the performance checkbox menu altogether. 
> 
> I'm still going to try & fix this, but if you have any tips, I really
> appreciate it.

Not sure, don't have a linux machine to test and XUL is mostly trial, error, and documentation for myself :\ pinging bgrins, he might know some XUL styling rules in this case.
Flags: needinfo?(jsantell) → needinfo?(bgrinstead)
I don't know what's going on specifically with the styling that's causing it to get messed up on Linux, but I can give a tip for debugging the problem:

* Open Browser Toolbox
* Open Inspector, enter #performance-filter-menupopup in the search menu and select the menupopup
* Then open the split console (ESC) and enter: $0.addEventListener("popuphiding", (e) => { e.preventDefault() })
* Then click the button to open the popup.  Notice that it doesn't go away when you focus back to the Browser Toolbox.  Hopefully now you will be able to dig into it's menuitem children and debug what's going on.

In this case it looks like there is a ::before element that is drawing the colored rectangle and on other platforms that's coming after the checkbox but for some reason it's not working on linux.  Maybe we should be styling the .menu-iconic-left element instead?  Not sure.
Flags: needinfo?(bgrinstead)
Hi!
I can reproduce this bug on ubuntu 14.04 & Firefox 43.0
If no one is working on it, then can I pick it up for my first contribution?

Thanks!
No updates in a few months, sending this over to you
Assignee: jonathan.wilfredo.fuentes → me
Hi Jordan,

I spent a lot of time on this, but didn't manage to fix it. The problem is with CSS & I am not good at it (it was my mistake that I picked it up).
I will pickup something else which would not be related to CSS. 

Thanks!
Thanks for the notice!
Assignee: me → nobody
I haven't tested this fix, but I think following these instructions should fix the bug:
- At [0], replace `#performance-filter-menupopup > menuitem:before` with `#performance-filter-menupopup > menuitem .menu-iconic-text::before`
- At [1], replace each `menuitem.marker-color-graphs-full-<color>:before` with `menuitem.marker-color-graphs-full-<color> .menu-iconic-text::before`


[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/performance.css#48
[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/performance.css#560
Attached patch performance.patch (obsolete) — Splinter Review
It works but it's little hacky way
(In reply to lukighostmail from comment #21)
> Created attachment 8703152 [details] [diff] [review]
> performance.patch
> 
> It works but it's little hacky way
Setting an arbitrary margin could potentially break other platforms. Can you try the solution suggested in comment 20 ?
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Priority: -- → P3
Summary: [Linux] The Filters menu places the checkbox behind the markers → The Filters menu places the checkbox behind the markers
Version: 40 Branch → unspecified
Mentor: jsantell
I applied the fix mentioned by Tim in comment #20. But on applying that only the checkboxes and their corresponding labels remained but the markers dissappeared.
Instead of moving the markers beside the checkboxes (which I tried but was unable to do) I moved the markers to the right of the labels.Is it allright?
Attachment #8765615 - Flags: review?(paul)
Attachment #8765615 - Flags: review?(paul) → review?(ntim.bugs)
Attachment #8703152 - Attachment is obsolete: true
Comment on attachment 8765615 [details] [diff] [review]
Moved the markers to the right of the checkbox labels.


> I applied the fix mentioned by Tim in comment #20. But on applying that only the checkboxes and
> their corresponding labels remained but the markers dissappeared.
Looks like .menu-iconic-text is a void element, so ::before/::after won't work on it.
How about the same instructions but with #performance-filter-menupopup > menuitem .menu-iconic-left::after ?


> Instead of moving the markers beside the checkboxes (which I tried but was unable to do)
> I moved the markers to the right of the labels.Is it allright?
The issue with this is that it makes it harder to see which colour goes with which label. It's also inconsistent with other places in the UI where we put the icon on the left of the label.
Attachment #8765615 - Flags: review?(ntim.bugs)
Attached patch Bug1172412.patchSplinter Review
Hi Tim, I applied the change which you suggested, now the marker no more appears over the checkbox, it appears beside the checkbox,before the label.
Attachment #8766829 - Flags: review?(ntim.bugs)
Comment on attachment 8765615 [details] [diff] [review]
Moved the markers to the right of the checkbox labels.

Looks good if it works on all platforms (Win, Linux) ! 

Tested on OSX, and it works fine.

Thanks for the patch!
Attachment #8765615 - Flags: review+
Comment on attachment 8765615 [details] [diff] [review]
Moved the markers to the right of the checkbox labels.

Whoops, wrong patch
Attachment #8765615 - Attachment is obsolete: true
Attachment #8765615 - Flags: review+
Attachment #8766829 - Flags: review?(ntim.bugs) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/c0edf3843275
Moved the markers to the right of the checkbox, before the label. r=ntim
https://hg.mozilla.org/mozilla-central/rev/c0edf3843275
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Attached image screenshot win10.png
Does this patch affect to Windows?
(In reply to magicp from comment #31)
> Created attachment 8767413 [details]
> screenshot win10.png
> 
> Does this patch affect to Windows?

Whoops, that is definitively unintended. Seems Win specific since osx isn't affected, and linux was fixed by this bug.
Can you file a new bug please ? Thanks !
Depends on: 1284433
I have seen that this bug is still not fixed in  latest Nightly 51.0a1 (2016-08-24) on Windows 7, 64 Bit!


Build ID   :  20160824030337

User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
 [testday-20160826]
(In reply to Maruf Rahman from comment #33)
> I have seen that this bug is still not fixed in  latest Nightly 51.0a1
> (2016-08-24) on Windows 7, 64 Bit!

This bug is about fixing linux.
Windows bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1284433
QA Whiteboard: [good first verify]
Fixed on Firefox 50.0b3 using Linux Mint 18
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.