Closed Bug 1254444 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox47 fixed, firefox48 verified)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- fixed
firefox48 --- verified

People

(Reporter: magicp.jp, Assigned: jsantell)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files)

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);
}
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Thanks for the report and regression range to boot!
Assignee: nobody → nfitzgerald
Keywords: regression
Priority: -- → P1
Thanks for the background info -- I'll grab this and get it uplifted to Dev Edition
Assignee: nfitzgerald → jsantell
Component: Developer Tools: Memory → Developer Tools: Shared Components
Attachment #8728693 - Flags: review?(nfitzgerald)
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
Attachment #8728693 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8728702 - Flags: review+
Attachment #8728702 - Flags: approval-mozilla-aurora?
(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
(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!
Flags: needinfo?(jsantell)
The leave-open was just for keeping this open until the aurora patch landed - sorry for the confusion!
Flags: needinfo?(jsantell)
https://hg.mozilla.org/mozilla-central/rev/f7abc73ebddb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
magicp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
Comment on attachment 8728702 [details] [diff] [review]
1254444-aurora.patch

CSS change, regression fix, taking it in Aurora47.
Attachment #8728702 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(magicp.jp)
(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!
Status: RESOLVED → VERIFIED
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.