1213100 - Refactor styles in memory tool

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

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

This'll be a good way to distribute workload on the memory tool, and someone faster than me can do some styles. It'd be rather easy, but in this case we're using xhtml instead of xul (so one step closer to HTML, but xhtml lets us transition easier), but we lose a lot of out-of-the-box styles we already have for XUL environments, so this could be an opportunity to start creating reusable widgets for non-XUL.

Many things in devtools/client/themes/memory.css will be tagged with this bug number to be cleaned up or generalized.

Comment 1

[Tim Nguyen :ntim]

See bug 1213137 comment 18 for more clean up comments.

Comment 2

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

More TODOs that I'm aware of:

* Entire panel scrolls (create more snapshots so it flows over), just snapshot area should scroll
* All the .heap-view-panel state messages need styling love -- they just describe the current state. Probably could use a loader.
* Maybe the default state should hide the snapshot bar, otherwise the default message will be off center
* If bug 960776 lands, the tree needs badly styled, but will be handled in follow up bug 1217239
* Dropdown breakdown menu maybe could look like our other dropdowns (console?)
* we'll be adding a checkbox with some text, or maybe we can just use the recording button from performance, so that may need something, in bug 1214066

Snapshot Item:
* Export (save?) button on the snapshot list item, like performance (functionality in bug 1215954)
* We'll have total bytes/count for each snapshot, maybe we can display that in a nice way? Like IE's tools: https://i-msdn.sec.s-msft.com/dynimg/IC769141.png

Buttons:
* "Take Snapshot" button in default view needs styled
* Clear button on the snapshot list item, like performance (functionality in bug 1215955)
* Import button on the main toolbar (probably?), like performance (functionality in bug 1215953)

Comment 3

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

Now used to refactor and clean the memory.css file, rather than small style changes.

Comment 5

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

Attachment: p1

Comment 6

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

(In reply to Victor Porof [:vporof][:vp] from comment #5)
> Created attachment 8680712 [details] [diff] [review]
> p1

This fixes bug 1219548 and bug 1219074, plus more crazy things.

Comment 7

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

Attachment: fix-selection-issue.patch

Comment 8

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

plz lnd

Comment 9

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

Once trees are up ;_;

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8680712 [details] [diff] [review]
p1

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

::: devtools/client/themes/memory.css
@@ +53,4 @@
>  .devtools-toolbar .devtools-button.take-snapshot {
> +  -moz-appearance: none;
> +  -moz-margin-start: 1px;
> +  -moz-margin-end: 1px;

nit: margin-inline-start and margin-inline-end

@@ +63,3 @@
>    background-size: 64px 16px;
>    background-position: 0 center;
>    background-repeat: no-repeat;

nit: you can remove the width, height and background-repeat properties, they are already defined in toolbars.inc.css

@@ +84,3 @@
>  .list {
> +  width: var(--sidebar-width);
> +  overflow-y: scroll;

nit: overflow-y: auto;
otherwise, the light theme on Windows will always have a scrollbar, even though there is no content to scroll. `auto` does some magic to show the scrollbar when needed.

@@ +89,1 @@
>    background-color: var(--theme-toolbar-background);

Can we use var(--theme-sidebar-background) ? After all, that variable was intended for use in sidebars.

@@ +89,2 @@
>    background-color: var(--theme-toolbar-background);
> +  -moz-border-end: 1px solid var(--theme-splitter-color);

nit: border-inline-end

@@ +228,5 @@
>  .heap-tree-item-total-bytes,
>  .heap-tree-item-total-count {
> +  text-align: end;
> +  -moz-border-end: var(--cell-border-color) 1px solid;
> +  -moz-padding-end: 5px;

nit: border-inline-end and padding-inline-end.

@@ +256,5 @@
>    background-position: -24px -24px;
> +  background-repeat: no-repeat;
> +  margin: 0px;
> +  margin-top: 2px;
> +  -moz-margin-end: 5px;

nit: margin-inline-end

Comment 11

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

Thanks for the comments, agree on all of them!

Comment 12

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

In the process of uplifting, any changes have to be filed in another bug (I did add ntim's comments in before starting, however)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e458fba06eb1

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=cb029edb917f

Comment 16

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

Attachment: p2

Clean up everything

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8681612 [details] [diff] [review]
p2

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

::: devtools/client/themes/memory.css
@@ +73,4 @@
>  }
>  
> +.devtools-toolbar > .devtools-button.take-snapshot {
> +  -moz-appearance: none;

You can get rid of this rule (HTML buttons don't need -moz-appearance: none, the styles in toolbars.inc.css are enough).

Comment 18

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

(In reply to Tim Nguyen [:ntim] from comment #17)
> Comment on attachment 8681612 [details] [diff] [review]
> p2
> 
> Review of attachment 8681612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/memory.css
> @@ +73,4 @@
> >  }
> >  
> > +.devtools-toolbar > .devtools-button.take-snapshot {
> > +  -moz-appearance: none;
> 
> You can get rid of this rule (HTML buttons don't need -moz-appearance: none,
> the styles in toolbars.inc.css are enough).

Thanks Tim!

Comment 19

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

(In reply to Victor Porof [:vporof][:vp] from comment #16)

There are many bugs filed for the memory tool about css. Most of them were fixed by the first patch attached in this bug, but then a bad rebase and a few improper changes were landed. The second patch in this bug addresses all of them.

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8681612 [details] [diff] [review]
p2

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

Looks good. Tested locally as discussed on IRC.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/74156ecb4a20

Comment 23

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

Nick, This should land on aurora ASAP. I'm currently cloning it, but it seems to go on forever. If you have an up to date aurora handy, can you please land this?

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=2021ea747bec

Comment 25

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

Thanks a bunch!

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74156ecb4a20

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Victor, Nick, could you please nominate this patch for uplift to Aurora? While this will miss the first Aurora44 build, I would like us to fix this soon.

Comment 28

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

(In reply to Ritu Kothari (:ritu) from comment #27)
> Victor, Nick, could you please nominate this patch for uplift to Aurora?
> While this will miss the first Aurora44 build, I would like us to fix this
> soon.

comment 24

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e458fba06eb1
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/74156ecb4a20

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/49d39d4df89b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2021ea747bec

Comment 31

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced the bug with Firefox Nightly 44.0a1 (2015-10-08);(Build ID: 20151008030232) on Linux, 64 Bit

This Bug is now verified as fixed on Latest Firefox Nightly 45.0a1 (2015-12-10);(Build ID: 20151210030212)

And also Latest Firefox Dev Edition 44.0a2 (2015-12-10); (Build ID: 20151210004006)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0