1285863 - Improve UI / UX of devtools JSONView

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Ruturaj Vartak]

Attachment: json-view-linux.png

The monospace font family in jsonview (linux) isn't loading correctly. Loading the sans-serif system default.
I suspect its https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#19

:root[platform="linux"] isn't working it seems, Firebug theme works well since its not using that selector.

The fonts are really tiny, There seems to be hardcoded font-size of 11px, 12px etc set. Shouldn't it be system specific (ie, inherited, etc?)

====================================
Tim's Suggestions


I'd be fine with changes in the Json viewer, but this bug doesn't seem like the right place. Please file a new one for it. I'd be happy to take a look at the changes.

:root[platform="linux"] is not working, because no script sets the attribute. Currently, theme-switching.js takes care of that, but it won't work when loaded in the Json viewer. One thing you can do is setting it from converter-child.js' toHtml function like this: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme-switching.js#11

==================================================

Comment 1

[Ruturaj Vartak]

Hey Tim, can I pick this?

Comment 2

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #1)
> Hey Tim, can I pick this?

Yes, please do. You can click (take) in the "Assigned To" field above to get assigned.

Comment 3

[Ruturaj Vartak]

Attachment: fix-1285863-1.patch

- Synced UX consistent with theme selected
- Synced Header view UX with Network Monitor > Header view
- Fixing some Tab issues (only checked on GNOME Ubuntu 16.04)
- Fixed, Firebug theme in tree viewer didn't have icons to expand or collapse tree

Comment 4

[Ruturaj Vartak]

Attachment: screenshots-jsonviewer-all-themes.zip

The zip file contains 9 screenshots based on attachment#8771736 [details] [diff] [review] (fix-1285863-1.patch)

- Firebug, Light and Dark Theme
- JSON Tree viewer Tab, Raw Data Tab, Header View Tab

Comment 5

[Ruturaj Vartak]

(In reply to Ruturaj Vartak from comment #3)
> Created attachment 8771736 [details] [diff] [review]
> fix-1285863-1.patch
> 
> - Synced UX consistent with theme selected
> - Synced Header view UX with Network Monitor > Header view
> - Fixing some Tab issues (only checked on GNOME Ubuntu 16.04)
> - Fixed, Firebug theme in tree viewer didn't have icons to expand or
> collapse tree

I forgot to add
- The size of the font in tree viewer is consistent with DOM panel

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8771736 [details] [diff] [review]
fix-1285863-1.patch

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

The code changes look fine. but UI-wise I see issues on OSX.

::: devtools/client/jsonview/converter-child.js
@@ +211,4 @@
>      let baseUrl = clientBaseUrl + "jsonview/";
>      let themeVarsUrl = clientBaseUrl + "themes/variables.css";
>      let commonUrl = clientBaseUrl + "themes/common.css";
> +  

nit: whitespace

@@ +215,5 @@
> +    let os;
> +    let platform = Services.appinfo.OS;
> +    if (platform.startsWith("Win")) {
> +      os = "win";
> +    } else if (platform.startsWith("Mac")) {

My OSX device is incorrectly detected as Linux. That's probably because `Services.appinfo.OS` provides different results than `navigator.platform`.

Services.appinfo.OS returns "Darwin" on my OSX machine. I *think* it returns WINNT on Windows.

::: devtools/client/jsonview/css/headers-panel.css
@@ +46,3 @@
>  
> +/* Theme colors have been generated / copied from Network Panel's
> + * Header view widgets 

nit: trailing whitespace

::: devtools/client/jsonview/css/main.css
@@ +24,5 @@
>  }
>  
>  /* The tree takes the entire horizontal space within the panel content. */
>  .panelContent .treeTable {
> +  font-size: 80%;

This seems to be making the font tiny on OSX (probably windows too)

::: devtools/client/jsonview/css/text-panel.css
@@ +14,3 @@
>    overflow: auto;
>    height: 100%;
> +  font-size: 80%;

This seems to be making the font tiny on OSX (probably windows too).

::: devtools/client/shared/components/tabs/tabs.css
@@ +112,5 @@
>  
>  .theme-dark .tabs .tab-panel,
>  .theme-light .tabs .tab-panel {
> +  /* Giving all available height, without it vertical scroll doesn't appear */
> +  height: 100%; 

nit: whitespace

::: devtools/client/themes/common.css
@@ +18,4 @@
>  
>  :root[platform="linux"],
>  :root.theme-firebug {
> +  --monospace-font-family: DejaVu Sans Mono, monospace;

Let's check with Helen and Honza about this change once all issues are resolved.

Comment 7

[Tim Nguyen :ntim]

Attachment: jsonview-issues-osx.png

Comment 8

[Ruturaj Vartak]

Attachment: fix-1285863-2.patch

Hey Tim, would it be possible for you to generate Mac and Windows screenshots ?
Here are the changes

- fixed trailing, leading whitespaces
- removed font-size: 80%
- adding firebug theme to high DPI
- fixed Services.appinfo.OS string expectations referring to [1]

[1] - https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Miscellaneous

Comment 9

[Ruturaj Vartak]

Attachment: 100-perc-font-size.png

This is JSON View with normal font-size without being set to 100%. Its bigger than DOM panel font size.

Comment 10

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #9)
> Created attachment 8771786 [details]
> 100-perc-font-size.png
> 
> This is JSON View with normal font-size without being set to 100%. Its
> bigger than DOM panel font size.

Can you try setting font: message-box on the <body> tag?

Comment 11

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #8)
> Created attachment 8771785 [details] [diff] [review]
> fix-1285863-2.patch
> 
> Hey Tim, would it be possible for you to generate Mac and Windows
> screenshots ?
Sure, will take a look later and publish screenshots.

Comment 12

[Tim Nguyen :ntim]

Attachment: osx-screen-1.png

Comment 13

[Tim Nguyen :ntim]

Attachment: osx-screen-2.png

Comment 14

[Tim Nguyen :ntim]

Attachment: osx-screen-3.png

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8771785 [details] [diff] [review]
fix-1285863-2.patch

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

Can you address the issues pointed in the screenshots ?

Also, to fix the font-size on linux, can you try setting font: message-box; on <body> in main.css ?

I've triggered a build on Windows so I can test it tomorrow and provide screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0185ac405ef4

::: devtools/client/shared/components/tree/tree-view.css
@@ +166,3 @@
>    .theme-dark .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon,
>    .theme-light .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon {
>      background-image: url("chrome://devtools/skin/images/[e-mail]");

Still doesn't fix Retina display on my side unfortunately :/ 

On Linux, you can test retina displays by zooming at 200% or by setting `layout.css.devPixelsPerPx` to 2 in about:config

Comment 16

[Ruturaj Vartak]

Attachment: fix-1285863-3.patch

- I've set font: message-box; in body (main.css), however font size was still big,
  I found this to sync linux monospace font, I've added the same
  // devtools/skin/common.css:32
  [platform="linux"]:root .devtools-monospace {
      font-size: 80%;
  }

- Fixed Raw data mismatch monospace font
- Fixed Retina display problems
  I had tried zooming, but it didn't help. However `layout.css.devPixelsPerPx`
  did help! Thanks for that
- Mismatching body backgrounds:
  Strange the color is same on my build
  I've added background-color: var(--theme-body-background)
  Lets see if it helps.

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8771862 [details] [diff] [review]
fix-1285863-3.patch

Still need to test the patch before I can set r+.

Honza, can you take a look to see if you're ok with the changes.

Comment 18

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #16)
> Created attachment 8771862 [details] [diff] [review]
> fix-1285863-3.patch
> 
> - I've set font: message-box; in body (main.css), however font size was
> still big,
>   I found this to sync linux monospace font, I've added the same
>   // devtools/skin/common.css:32
>   [platform="linux"]:root .devtools-monospace {
>       font-size: 80%;
>   }
Sounds good.

> - Fixed Raw data mismatch monospace font
> - Fixed Retina display problems
>   I had tried zooming, but it didn't help. However
> `layout.css.devPixelsPerPx`
>   did help! Thanks for that
The fix works!

> - Mismatching body backgrounds:
>   Strange the color is same on my build
>   I've added background-color: var(--theme-body-background)
>   Lets see if it helps.
The "JSON" and "Headers" tabs are still not fixed.

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8771862 [details] [diff] [review]
fix-1285863-3.patch

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

::: devtools/client/jsonview/css/text-panel.css
@@ +21,4 @@
>  }
>  
> +:root[platform="linux"] .textPanelBox .data {
> +	font-size: 80%;

nit: fix indentation

::: devtools/client/shared/components/tree/tree-view.css
@@ +166,2 @@
>    .theme-dark .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon,
>    .theme-light .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon {

These 3 selectors can be simplified to: .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon

Comment 20

[Ruturaj Vartak]

Attachment: fix-1285863-4.patch

- fixing nit
- optimized CSS as suggested
- Another go at syncing background color

Comment 21

[Ruturaj Vartak]

Attachment: css-property-not-cascading.png

Comment 22

[Ruturaj Vartak]

Comment on attachment 8773149 [details]
css-property-not-cascading.png

Hey Tim,
The background issue: The attachment#8773149 [details] possibly shows a reason. I don't know whether its a bug in devtools, that it isn't showing the cascading property or actually the property is not cascading downwards.

Comment 23

[Tim Nguyen :ntim]

La (In reply to Ruturaj Vartak from comment #22)
> Comment on attachment 8773149 [details]
> css-property-not-cascading.png
> 
> Hey Tim,
> The background issue: The attachment#8773149 [details] possibly shows a
> reason. I don't know whether its a bug in devtools, that it isn't showing
> the cascading property or actually the property is not cascading downwards.

The property is not supposed to inherit. By default, any element has background: transparent; no matter what background the parent has.

Comment 24

[Ruturaj Vartak]

ok, has the new patch managed to fix the Mac background-color issue ? In Mac, somehow the background color of the toolbar is being used !! and I'm unable to replicate that on my Linux box

Comment 25

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #24)
> ok, has the new patch managed to fix the Mac background-color issue ? In
> Mac, somehow the background color of the toolbar is being used !! and I'm
> unable to replicate that on my Linux box

Will test this out later today.

Comment 26

[Tim Nguyen :ntim]

Comment on attachment 8773148 [details] [diff] [review]
fix-1285863-4.patch

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

This still doesn't fix the problem locally, this is how it looks for me: http://imgur.com/a/UavIH

Perhaps you should rebase your patch upon latest master, it might be the reason you can't reproduce the issue.

::: devtools/client/shared/components/tabs/tabs.css
@@ +107,4 @@
>  .theme-light .tabs .tabs-navigation {
>    border-bottom: 1px solid var(--theme-splitter-color);
>    box-shadow: 0 -2px 0 rgba(170, 170, 170,.1) inset;
> +  font: message-box;

font: message-box is not needed here (since it's already set on body).

Comment 27

[Ruturaj Vartak]

Alrite, sync master onto my branch and see if it helps me pin point the problem

Comment 28

[Ruturaj Vartak]

I'm Sorry, i meant "let me sync my master"

Comment 29

[Ruturaj Vartak]

Attachment: fix-1285863-5.patch

Comment 30

[Ruturaj Vartak]

Attachment: fix-1285863-5.patch

- Forked out from mozilla/master @4969042
- Tabs: fonts, padding in sync with main devtools tabs
- Toolbar buttons: keeping in sync with Network monitor buttons

Comment 31

[Ruturaj Vartak]

Attachment: fix-1285863-6.patch

- additionally fixing :hover color for theme-dark

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8773778 [details] [diff] [review]
fix-1285863-6.patch

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

::: devtools/client/jsonview/css/main.css
@@ +15,5 @@
>  @import "headers-panel.css";
>  
> +.jsonPanelBox {
> +  height: 100%;
> +}

I don't like this fix, it still leaves a small section without the right background at the bottom of the page.
Instead, can you move background-color: var(--tab-toolbar-background) from .tabs to .tabs-navigation ?

@@ +26,2 @@
>    padding-right: 5px;
> +  background-color: var(--theme-body-background);

With my suggested fix, this is no longer needed.

::: devtools/client/jsonview/css/search-box.css
@@ +7,4 @@
>  /* Search Box */
>  
>  .searchBox {
> +  height: 20px;

Why this change ? If this is to get the searchbox horizontally aligned, I'd prefer you to change position: fixed; right: 1px; to float: right; then add some margin around the textbox.

::: devtools/client/jsonview/css/toolbar.css
@@ +8,4 @@
>  
>  .toolbar {
>    line-height: 20px;
> +  height: 24px;

Can you restore the old height ? It's more consistent with the toolbox toolbar height (at least for me).

@@ +24,4 @@
>    vertical-align: middle;
>    cursor: pointer;
>    -moz-user-select: none;
> +  padding: 0 2px 0px 2px;

nit: padding: 0 2px;

::: devtools/client/shared/components/tabs/tabs.css
@@ +74,4 @@
>  .theme-dark .tabs .tabs-menu-item a,
>  .theme-light .tabs .tabs-menu-item a:hover,
>  .theme-light .tabs .tabs-menu-item a {
> +  padding: 0px 15px;

The new padding makes the tabs much smaller than the toolbox tabs. Can you undo this change ?

Comment 33

[Ruturaj Vartak]

Attachment: toolbar-height.png

In linux the 22px height looks like this, falling a short of 2px.

Could you please share Mac screenshot in the existing state (22px height) ?

Comment 34

[Ruturaj Vartak]

Attachment: tab-sizes.png

The tabs padding if reverted looks bigger in Linux. Could you please share mac screenshots?

Comment 35

[Ruturaj Vartak]

Attachment: fix-1285863-7.patch

Following is the summary of changes

- .jsonPanelBox needs height:100% (without it scrollbar doesn't appear for long big JSONs)
  similar CSS is already present in .headersPanelBox and .textPanelBox
- moved "background-color: var(--tab-toolbar-background) from .tabs to .tabs-navigation" as suggested
- now even minimizing entire JSON, still makes its *active* area have right background
- Reverting buttons / tab sizes
- .searchBox now using float: right (why would we prefer float:right vs positon: fixed ?)
- fixing: arrows not visible in firebug theme
- fixing: high DPI arrows in tree-view
- fixing: last-child tab didn't have a border

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8774106 [details] [diff] [review]
fix-1285863-7.patch

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

The changes look fine, the light and dark themes look good. Though the Firebug theme has issues.

::: devtools/client/shared/components/tabs/tabs.css
@@ +38,4 @@
>  .tabs .tab-panel-box,
>  .tabs .tab-panel {
>    height: 100%;
> +  background-color: var(--theme-body-background);

You can omit this I think since it's already applied on `body`.

@@ +61,5 @@
>    border-color: var(--theme-splitter-color);
>  }
>  
> +.theme-dark .tabs .tabs-menu-item:last-child,
> +.theme-light .tabs .tabs-menu-item:last-child {

Use .theme-light:not(.theme-firebug) to avoid affecting the Firebug theme (the fb theme has both .theme-light and .theme-firebug applied).

Comment 37

[Tim Nguyen :ntim]

Attachment: dark theme

Comment 38

[Tim Nguyen :ntim]

Attachment: Firebug theme

Comment 39

[Ruturaj Vartak]

Attachment: fix-1285863-8.patch

- Fixed firebug-theme tab color in inspector
- Fixed last child firebug tab border issue
- Fixed firebug-theme active tab's border-color

Comment 40

[Ruturaj Vartak]

Attachment: without-tab-panel-box-background.png

>  .tabs .tab-panel-box,
>  .tabs .tab-panel {
>    height: 100%;
> +  background-color: var(--theme-body-background);

Above code is required, without it this is how it looks in firebug theme.

Comment 41

[Tim Nguyen :ntim]

Helen, Are you ok with changing the default font on linux and the firebug theme to DejaVu Sans?

Comment 42

[Tim Nguyen :ntim]

Wait this change was removed in the latest patch

Comment 43

[Tim Nguyen :ntim]

Comment on attachment 8774383 [details] [diff] [review]
fix-1285863-8.patch

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

Sorry for the delay Raturaj, can you fix the conflicts on top of fx-team ?

::: devtools/client/jsonview/css/main.css
@@ +15,5 @@
>  @import "headers-panel.css";
>  
> +.jsonPanelBox {
> +  height: 100%;
> +}

can you move this to json-panel.css ?

Comment 44

[Ruturaj Vartak]

Attachment: broken-tabs-firebug-theme.png

Comment 45

[Ruturaj Vartak]

Hey Tim,

I merged fx-team with my branch. And things are not great as were in the master, here are the following issues

- filter.svg isn't same as in "master" branch. (logo doesn't seem to be right one, its darker, not the svg we fixed in bug#1253195 [1]
- Firebug theme Tabs seem to be broken (bottom line of tabs not aligning with the toolbar) attachment#8776245 [details]

I've generally merged from "master" branch, "fx-team" looks "behind", I'm not sure how it moves in mozilla. All the diffs / patches that I've submitted have been with "master". So I'm confused, how should I go about fixing this ? Should I re-work on Firebug theme? 

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1253195&attachment=8769411

Comment 46

[Ruturaj Vartak]

I forgot to mention, I merged the fx-team @ 70ec49d8e

Comment 47

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #45)
> Hey Tim,
> 
> I merged fx-team with my branch. And things are not great as were in the
> master, here are the following issues
> 
> - filter.svg isn't same as in "master" branch. (logo doesn't seem to be
> right one, its darker, not the svg we fixed in bug#1253195 [1]
This change was intentional, it was meant for the toolbar icons. Use filter.svg#filterinput to get the old color.

> - Firebug theme Tabs seem to be broken (bottom line of tabs not aligning
> with the toolbar) attachment#8776245 [details]
There has been many fixes for tabs (rtl related, active states, font,...) in the last week.
It's possible those change broke the JSON viewer, or are incompatible with your changes.
Can you fix the firebug theme tabs ?

> I've generally merged from "master" branch, "fx-team" looks "behind", I'm
> not sure how it moves in mozilla. All the diffs / patches that I've
> submitted have been with "master". So I'm confused, how should I go about
> fixing this ? Should I re-work on Firebug theme?
fx-team is where all of our changes arrive first. That means it's more up-to-date. After a day, those changes get merged to master.


> [1]
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=1253195&attachment=8769411

Comment 48

[Tim Nguyen :ntim]

Ruturaj, I've fixed the JSON Viewer issues you were seeing on fx-team. Can you try rebasing again ?
Sorry for the trouble.

Comment 49

[Ruturaj Vartak]

Attachment: fix-1285863-9.patch

- Fixes after merging mozilla/fx-team @70ec49d8

I'm finding a specific issue, if we click on "JSON" tab and then go to either the "Headers" or "Raw Data" tab, I see some extra vertical white space, I'm unable to find what it is, its not padding, margin. Its not any absolute spacing either. Could you please help me understand why its happening. This only happens if we click "JSON" tab. Otherwise everything is fine.

Comment 50

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #49)
> Created attachment 8776284 [details] [diff] [review]
> fix-1285863-9.patch
> 
> - Fixes after merging mozilla/fx-team @70ec49d8
> 
> I'm finding a specific issue, if we click on "JSON" tab and then go to
> either the "Headers" or "Raw Data" tab, I see some extra vertical white
> space, I'm unable to find what it is, its not padding, margin. Its not any
> absolute spacing either. Could you please help me understand why its
> happening. This only happens if we click "JSON" tab. Otherwise everything is
> fine.

Will take a look.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4bd1976fbb4a
Improve UI / UX of devtools JSONView r=ntim

Comment 52

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #49)
> Created attachment 8776284 [details] [diff] [review]
> fix-1285863-9.patch
> 
> - Fixes after merging mozilla/fx-team @70ec49d8
> 
> I'm finding a specific issue, if we click on "JSON" tab and then go to
> either the "Headers" or "Raw Data" tab, I see some extra vertical white
> space, I'm unable to find what it is, its not padding, margin. Its not any
> absolute spacing either. Could you please help me understand why its
> happening. This only happens if we click "JSON" tab. Otherwise everything is
> fine.

Fixed and merged with fx-team. Should be in Nightly tomorrow or so.

Comment 53

[Ruturaj Vartak]

> Fixed and merged with fx-team. Should be in Nightly tomorrow or so.

Hey Tim,
Thanks. Could u tell me what the issue was?

Comment 54

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #53)
> > Fixed and merged with fx-team. Should be in Nightly tomorrow or so.
> 
> Hey Tim,
> Thanks. Could u tell me what the issue was?

The bigger padding on the tabs was the issue.

Comment 55

[Tim Nguyen :ntim]

You can check the changes I've made here: https://hg.mozilla.org/integration/fx-team/diff/4bd1976fbb4a/devtools/client/shared/components/tabs/tabs.css

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bd1976fbb4a

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/331c4166a3a2
Improve UI / UX of devtools JSONView: Remove unused variable 'code'. r=jryans on IRC a=eslint-fix