1172412 - The Filters menu places the checkbox behind the markers

Status: VERIFIED FIXED

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

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Ubuntu 14.04 (x64)

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.

Comment 1

[Andrei Vaida [:avaida]]

Attachment: screenshot from Ubuntu 14.04 (x64) #2

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Wonder if related to bug 1172737

Comment 3

[Neil Deakin]

Likely fixed by 1172282

Comment 4

[Neil Deakin]

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.

Comment 5

[Andrei Vaida [:avaida]]

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

Comment 6

[Jonathan [:elefont]]

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!

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Hey Jonathan, sorry for the delay -- are you still interested in this bug?

Comment 8

[Jonathan [:elefont]]

Hey Jordan, yeah I'm still interested. Although I'm not entirely sure where to start.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 10

[Jonathan [:elefont]]

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.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Hey Jonathan, any updates? Anything I can help with?

Comment 12

[Jonathan [:elefont]]

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 :)

Comment 13

[Jonathan [:elefont]]

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.

Comment 14

[Jordan Santell [:jsantell] [@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.

Comment 15

[Brian Grinstead [:bgrins]]

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.

Comment 16

[me]

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!

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

No updates in a few months, sending this over to you

Comment 18

[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!

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Thanks for the notice!

Comment 20

[Tim Nguyen :ntim]

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

Comment 21

[lukighostmail]

Attachment: performance.patch

It works but it's little hacky way

Comment 22

[Tim Nguyen :ntim]

(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 ?

Comment 23

[Victor Porof [:vporof][:vp]]

Triaging. Filter on ADRENOCORTICOTROPIC (yes).

Comment 24

[Deepjyoti Mondal]

Attachment: 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.
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?

Comment 25

[Tim Nguyen :ntim]

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.

Comment 26

[Deepjyoti Mondal]

Attachment: Bug1172412.patch

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.

Comment 27

[Tim Nguyen :ntim]

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!

Comment 28

[Tim Nguyen :ntim]

Comment on attachment 8765615 [details] [diff] [review]
Moved the markers to the right of the checkbox labels.

Whoops, wrong patch

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c0edf3843275
Moved the markers to the right of the checkbox, before the label. r=ntim

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0edf3843275

Comment 31

[magicp]

Attachment: screenshot win10.png

Does this patch affect to Windows?

Comment 32

[Tim Nguyen :ntim]

(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 !

Comment 33

[Mohammad Maruf Rahman]

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]

Comment 34

[Tim Nguyen :ntim]

(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

Comment 35

[Subhrajyoti Sen]

Fixed on Firefox 50.0b3 using Linux Mint 18