1254444 - [DevTools][Memory] frame-links are not enabled focused color

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[magicp]

Attachment: devtools-memory-focused-color.png

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

Steps to reproduce:

1. Start Nightly
2. Go to "about:home"
3. Open DevTools > Memory
4. Check-on "Record allocation stacks"
5. Reload current page
6. Switch group-by from "Type" to "Call Stack"
7. Take snapshot
8. Select any rows from heap-view


Actual results:

frame-links are not enabled focused color. This bug was fixed once by Bug 1242646.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=5cb797a911407cbddecd4f0c347ca0da251cff7a&tochange=9b3c9c05e11c7406969b9f4246f83a0422e084ed


Expected results:

Focused color should be enabled.

components-frame.css - line:15, should move to bottom.

.focused .frame-link-filename,
.focused .frame-link-column,
.focused .frame-link-line,
.focused .frame-link-host,
.focused .frame-link-colon {
  color: var(--theme-selection-color);
}

Comment 1

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

Thanks for the report and regression range to boot!

Comment 2

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

Thanks for the background info -- I'll grab this and get it uplifted to Dev Edition

Comment 3

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

Attachment: 1254444-focus-frame.patch

Comment 4

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

Comment on attachment 8728693 [details] [diff] [review]
1254444-focus-frame.patch

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

r=me assuming you tested this locally and verified the selected row is legible

Comment 5

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

Attachment: 1254444-aurora.patch

Separate patch for Aurora for clean landing:

Approval Request Comment
[Feature/regressing bug #]: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=5cb797a911407cbddecd4f0c347ca0da251cff7a&tochange=9b3c9c05e11c7406969b9f4246f83a0422e084ed
[User impact if declined]: Difficult to read source names in the memory tool
[Describe test coverage new/current, TreeHerder]: CSS change
[Risks and why]: If we don't land, users will have difficulty reading source names in the memory tool, and looks rather unpolished.

Comment 6

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

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> Comment on attachment 8728693 [details] [diff] [review]
> 1254444-focus-frame.patch
> 
> Review of attachment 8728693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me assuming you tested this locally and verified the selected row is
> legible

Good thing too, because the previous patch just landed caused the anchor selector to take precedent over the focused classes

Comment 7

[Pulsebot]

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

Comment 8

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

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #6)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> > Comment on attachment 8728693 [details] [diff] [review]
> > 1254444-focus-frame.patch
> > 
> > Review of attachment 8728693 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me assuming you tested this locally and verified the selected row is
> > legible
> 
> Good thing too, because the previous patch just landed caused the anchor
> selector to take precedent over the focused classes

Jordan, does this mean that this fix needs more work? I also noticed the "leave-open" keyword. Will there be a part 2? If yes, I'd much rather uplift "the good final" fix in one go. Thanks!

Comment 9

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

The leave-open was just for keeping this open until the aurora patch landed - sorry for the confusion!

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

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

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 8728702 [details] [diff] [review]
1254444-aurora.patch

CSS change, regression fix, taking it in Aurora47.

Comment 13

[magicp]

Attachment: Bug-1254444-verified-48.0a1.png

(In reply to Ritu Kothari (:ritu) from comment #11)
> magicp, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks!

This issue was fixed on Nightly. Thanks.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13f341508044

Comment 15

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

(In reply to magicp from comment #13)
> Created attachment 8730382 [details]
> Bug-1254444-verified-48.0a1.png
> 
> (In reply to Ritu Kothari (:ritu) from comment #11)
> > magicp, could you please verify this issue is fixed as expected on a latest
> > Nightly build? Thanks!
> 
> This issue was fixed on Nightly. Thanks.

Great!

Comment 16

[Forhad Hossain]

I have reproduced this bug with Nightly 48.0a1 (2016-03-08)  with instruction from comment 0 and Linux 32 Bit.     

Verified as fixed with Latest Developer Edition 47.0a2 (2016-04-16) (Build ID:20160416004025)

User Agent : Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0

[testday-20160415]