Closed Bug 1426057 Opened 2 years ago Closed 2 years ago

Netmonitor Performance Analysis styling is incorrect after removing widgets.css

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 verified)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- verified

People

(Reporter: magicp.jp, Assigned: rickychien)

References

Details

Attachments

(2 files)

Steps to reproduce:
1. Launch Nightly
2. Open Netmonitor
3. Go to any sites
4. Start performance analysis

Actual results:
Performance analysis styling is incorrect.

Expected results:
Same styling with Firefox 58.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4e2cf0afd644cc0f2dfb0f6def3f1c1d25a19d27&tochange=15c9aea2931fbaeceb4467097e09a565f6b706ee
Blocks: 1424916
Has Regression Range: --- → yes
Has STR: --- → yes
Thanks for the report!

Ricky, looks like there are some styles we need in widgets.css (Bug 1424916)

Honza
Flags: needinfo?(rchien)
Priority: -- → P2
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

https://reviewboard.mozilla.org/r/209238/#review215078

1) chrome://devtools/skin/widgets.css has ~1300 lines
2) chrome://devtools/content/shared/widgets/widgets.css has ~100 lines

So, by removing #2 we don't save much.

Could we rather identify what CSS rules Net panel needs from #1, put them into separate file and include only that new file?

Honza
Attachment #8938614 - Flags: review?(odvarko) → review-
Yep, nice idea. Only 110 lines we need from skin/widgets.css can make the chart works. This is better than loading entire 1300 lines skin/widgets.css and reducing a bit load time.
Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

https://reviewboard.mozilla.org/r/209238/#review215366

Nice, can we remove all the Charts styles from widgets.css now?

Honza
Attachment #8938614 - Flags: review?(odvarko) → review-
https://searchfox.org/mozilla-central/source/devtools/shared/gcli/commands/csscoverage.js#13

csscoverage.js (Style Editor) is using Chart.js, which will require widgets.css for charts styling. Or maybe we can move charts.css from widgets.css instead of copying it to netmonitor (but it will require to modify Style Editor codebase). Otherwise, I'd prefer to just revert the widgets.css changes like we did in Comment 2.

What do you think?
Flags: needinfo?(odvarko)
My preferred solution would be splitting out creating a new charts.css out of widgets.css then importing charts.css in netmonitor.css and styleeditor.css
(In reply to Ricky Chien [:rickychien] from comment #7)
> Or maybe we can move charts.css from widgets.css instead of copying it to netmonitor
Yes, this sound like the right option for me too.

Honza
Flags: needinfo?(odvarko)
Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

https://reviewboard.mozilla.org/r/209238/#review215716

Looks great, thanks Ricky!

Honza
Attachment #8938614 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff41bc984640
Move chart style from skin/widgets.css StatisticsPanel.css r=Honza
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/b2d620dbc30c
Move chart style from skin/widgets.css StatisticsPanel.css r=Honza
https://hg.mozilla.org/mozilla-central/rev/b2d620dbc30c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
This bug fix has verified in the latest Nightly build (20180104220114). Thanks!
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.