Closed Bug 1266310 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 verified)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: magicp.jp, Assigned: Honza)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Attached patch bug1266310.patch (obsolete) — Splinter Review
@jryans: just two line-change

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8744903 - Flags: review?(jryans)
Comment on attachment 8744903 [details] [diff] [review]
bug1266310.patch

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

Sending memory tool to :fitzgen.
Attachment #8744903 - Flags: review?(jryans) → review?(nfitzgerald)
Attachment #8744903 - Flags: review?(nfitzgerald) → review+
Attached patch bug1266310.patch (obsolete) — Splinter Review
Thanks Nick, I am attaching new patch with updated commit message (reviewer).

Honza
Attachment #8744903 - Attachment is obsolete: true
Attachment #8744924 - Flags: review+
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?
Attached patch bug1266310.patchSplinter Review
(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
Attachment #8744924 - Attachment is obsolete: true
Attachment #8745341 - Flags: review?(ntim.bugs)
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?
Attachment #8745341 - Flags: review?(ntim.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/146e4f2558c9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
Attachment #8745341 - Flags: approval-mozilla-aurora?
Hello magicp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
Comment on attachment 8745341 [details] [diff] [review]
bug1266310.patch

Firebug theme is a planned feature for Fx48, let's uplift to Aurora.
Attachment #8745341 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This issue is fixed as expected on a latest Nightly build. Thanks!
Flags: needinfo?(magicp.jp)
(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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.