1426057 - Netmonitor Performance Analysis styling is incorrect after removing widgets.css

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[magicp]

Attachment: performance-analysis-styling.png

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

Comment 1

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

Thanks for the report!

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

Honza

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

Review commit: https://reviewboard.mozilla.org/r/209238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209238/

Comment 3

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

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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209238/diff/1-2/

Comment 5

[Ricky Chien [:rickychien] (inactive)]

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 6

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

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 8

[Tim Nguyen :ntim]

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

Comment 9

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

(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

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209238/diff/2-3/

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8938614 [details]
Bug 1426057 - Move chart style from skin/widgets.css StatisticsPanel.css

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209238/diff/3-4/

Comment 12

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

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff41bc984640
Move chart style from skin/widgets.css StatisticsPanel.css r=Honza

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b2d620dbc30c
Move chart style from skin/widgets.css StatisticsPanel.css r=Honza

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[magicp]

This bug fix has verified in the latest Nightly build (20180104220114). Thanks!

Comment 17

[Tiberius Oros[:tiberius_oros]]

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