Closed Bug 1285863 Opened 4 years ago Closed 4 years ago

Improve UI / UX of devtools JSONView

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: ruturaj, Assigned: ruturaj, Mentored)

References

(Depends on 1 open bug)

Details

Attachments

(8 files, 16 obsolete files)

490.62 KB, application/zip
Details
19.63 KB, image/png
Details
69.12 KB, image/png
Details
260.76 KB, image/png
Details
386.37 KB, image/png
Details
22.87 KB, image/png
Details
45.27 KB, image/png
Details
12.16 KB, patch
ntim
: review+
Details | Diff | Splinter Review
Attached image json-view-linux.png (obsolete) —
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

==================================================
Hey Tim, can I pick this?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Assignee: nobody → ruturaj
Attached patch fix-1285863-1.patch (obsolete) — Splinter Review
- 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
Attachment #8771736 - Flags: review?(ntim.bugs)
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
(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 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.
Attachment #8771736 - Flags: review?(ntim.bugs)
Attached image jsonview-issues-osx.png (obsolete) —
Attached patch fix-1285863-2.patch (obsolete) — Splinter Review
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
Attachment #8771736 - Attachment is obsolete: true
Attachment #8771785 - Flags: review?(ntim.bugs)
Attached image 100-perc-font-size.png (obsolete) —
This is JSON View with normal font-size without being set to 100%. Its bigger than DOM panel font size.
(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?
(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.
Attached image osx-screen-1.png (obsolete) —
Attached image osx-screen-2.png (obsolete) —
Attached image osx-screen-3.png (obsolete) —
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/controls@2x.png");

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
Attachment #8771785 - Flags: review?(ntim.bugs)
Attached patch fix-1285863-3.patch (obsolete) — Splinter Review
- 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.
Attachment #8771785 - Attachment is obsolete: true
Attachment #8771862 - Flags: review?(ntim.bugs)
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.
Attachment #8771862 - Flags: feedback?(odvarko)
(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 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
Attachment #8771862 - Flags: review?(ntim.bugs)
Attached patch fix-1285863-4.patch (obsolete) — Splinter Review
- fixing nit
- optimized CSS as suggested
- Another go at syncing background color
Attachment #8771862 - Attachment is obsolete: true
Attachment #8771862 - Flags: feedback?(odvarko)
Attachment #8773148 - Flags: review?(ntim.bugs)
Attached image css-property-not-cascading.png (obsolete) —
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.
Flags: needinfo?(ntim.bugs)
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.
Flags: needinfo?(ntim.bugs)
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
(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 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).
Attachment #8773148 - Flags: review?(ntim.bugs)
Alrite, sync master onto my branch and see if it helps me pin point the problem
I'm Sorry, i meant "let me sync my master"
Attached patch fix-1285863-5.patch (obsolete) — Splinter Review
Attached patch fix-1285863-5.patch (obsolete) — Splinter Review
- Forked out from mozilla/master @4969042
- Tabs: fonts, padding in sync with main devtools tabs
- Toolbar buttons: keeping in sync with Network monitor buttons
Attachment #8773148 - Attachment is obsolete: true
Attachment #8773767 - Attachment is obsolete: true
Attachment #8773768 - Flags: review?(ntim.bugs)
Attached patch fix-1285863-6.patch (obsolete) — Splinter Review
- additionally fixing :hover color for theme-dark
Attachment #8773768 - Attachment is obsolete: true
Attachment #8773768 - Flags: review?(ntim.bugs)
Attachment #8773778 - Flags: review?(ntim.bugs)
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 ?
Attachment #8773778 - Flags: review?(ntim.bugs) → review-
Attached image 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) ?
Attached image tab-sizes.png
The tabs padding if reverted looks bigger in Linux. Could you please share mac screenshots?
Flags: needinfo?(ntim.bugs)
Attached patch fix-1285863-7.patch (obsolete) — Splinter Review
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
Attachment #8773778 - Attachment is obsolete: true
Attachment #8774106 - Flags: review?(ntim.bugs)
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).
Attachment #8774106 - Flags: review?(ntim.bugs) → review+
Attached image dark theme
Attachment #8769557 - Attachment is obsolete: true
Attachment #8771769 - Attachment is obsolete: true
Attachment #8771786 - Attachment is obsolete: true
Attachment #8771803 - Attachment is obsolete: true
Attachment #8771804 - Attachment is obsolete: true
Attachment #8771805 - Attachment is obsolete: true
Attachment #8773149 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attached image Firebug theme
Attached patch fix-1285863-8.patch (obsolete) — Splinter Review
- Fixed firebug-theme tab color in inspector
- Fixed last child firebug tab border issue
- Fixed firebug-theme active tab's border-color
Attachment #8774383 - Flags: review?(ntim.bugs)
Attachment #8774106 - Attachment is obsolete: true
>  .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.
Helen, Are you ok with changing the default font on linux and the firebug theme to DejaVu Sans?
Flags: needinfo?(hholmes)
Wait this change was removed in the latest patch
Flags: needinfo?(hholmes)
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 ?
Attachment #8774383 - Flags: review?(ntim.bugs) → review+
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
Flags: needinfo?(ntim.bugs)
I forgot to mention, I merged the fx-team @ 70ec49d8e
(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
Flags: needinfo?(ntim.bugs)
Ruturaj, I've fixed the JSON Viewer issues you were seeing on fx-team. Can you try rebasing again ?
Sorry for the trouble.
- 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.
Attachment #8774383 - Attachment is obsolete: true
Attachment #8776284 - Flags: review?(ntim.bugs)
Attachment #8776284 - Flags: review?(ntim.bugs) → review+
(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.
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/4bd1976fbb4a
Improve UI / UX of devtools JSONView r=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.
Flags: needinfo?(ntim.bugs)
> Fixed and merged with fx-team. Should be in Nightly tomorrow or so.

Hey Tim,
Thanks. Could u tell me what the issue was?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/4bd1976fbb4a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Pushed by archaeopteryx@coole-files.de:
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
Depends on: 1328009
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.