1266310 - Firebug theme - heap-tree select row is overlapped next row

Status: VERIFIED FIXED

(DevTools :: Memory, defect)

Description

[magicp]

Attachment: incorrect-row-height.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160420030213

Steps to reproduce:

1. Start latest Nightly
2. Open DevTools with Firebug theme > Memory
3. Switch view to "Aggregate"
4. Take snapshot
5. Select any row in heap-tree


Actual results:

heap-tree select row is overlapped next row.


Expected results:

heap-tree select row does not overlap next row.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266310.patch

@jryans: just two line-change

Honza

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8744903 [details] [diff] [review]
bug1266310.patch

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

Sending memory tool to :fitzgen.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266310.patch

Thanks Nick, I am attaching new patch with updated commit message (reviewer).

Honza

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8744924 [details] [diff] [review]
bug1266310.patch

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

::: devtools/client/themes/memory.css
@@ +530,5 @@
>  }
>  
> +/* Override Firebug styles for toolbar buttons to fix entire row height. */
> +.theme-firebug .heap-tree-item-individuals > button {
> +  margin: 0 auto;

I would rather add !important in the rule above, and add a comment in the block above.

@@ +531,5 @@
>  
> +/* Override Firebug styles for toolbar buttons to fix entire row height. */
> +.theme-firebug .heap-tree-item-individuals > button {
> +  margin: 0 auto;
> +  min-height: inherit;

Any reason why the min-height is needed in the first place?

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1266310.patch

(In reply to Tim Nguyen :ntim from comment #4)
> Comment on attachment 8744924 [details] [diff] [review]
> bug1266310.patch
> 
> Review of attachment 8744924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/memory.css
> @@ +530,5 @@
> >  }
> >  
> > +/* Override Firebug styles for toolbar buttons to fix entire row height. */
> > +.theme-firebug .heap-tree-item-individuals > button {
> > +  margin: 0 auto;
> 
> I would rather add !important in the rule above, and add a comment in the
> block above.
Done

> 
> @@ +531,5 @@
> >  
> > +/* Override Firebug styles for toolbar buttons to fix entire row height. */
> > +.theme-firebug .heap-tree-item-individuals > button {
> > +  margin: 0 auto;
> > +  min-height: inherit;
> 
> Any reason why the min-height is needed in the first place?
I removed it and it looks good.

Honza

Comment 6

[Pulsebot]

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

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8745341 [details] [diff] [review]
bug1266310.patch

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

Seems like the other patch landed in fx-team [0], so you'll probably need to make this a follow up patch.

[0]: https://hg.mozilla.org/integration/fx-team/rev/146e4f2558c9

Anyway, this looks good to me, thanks for addressing my comments!

::: devtools/client/themes/memory.css
@@ +524,5 @@
>  
>  .heap-tree-item-individuals > button {
>    height: 10px;
>    width: 32px;
> +  margin: 0 auto !important;

Can you add a comment on why !important is needed?

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/146e4f2558c9

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Follow up bug 1267998

Honza

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8745341 [details] [diff] [review]
bug1266310.patch

Approval Request Comment
[Feature/regressing bug #]: Firebug theme
[User impact if declined]: See attachment 8743687 [details]
[Describe test coverage new/current, TreeHerder]: in Nightly
[Risks and why]: Low, CSS only
[String/UUID change made/needed]: none

// note that this patch includes bug 1267998 as well

Comment 11

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

Hello magicp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 12

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

Comment on attachment 8745341 [details] [diff] [review]
bug1266310.patch

Firebug theme is a planned feature for Fx48, let's uplift to Aurora.

Comment 13

[magicp]

Attachment: Bug 1266310 - verified.png

This issue is fixed as expected on a latest Nightly build. Thanks!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f5b90356262

Comment 15

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

(In reply to magicp from comment #13)
> Created attachment 8748368 [details]
> Bug 1266310 - verified.png
> 
> This issue is fixed as expected on a latest Nightly build. Thanks!

Awesome! Thanks for the verification.