836233 - [Inspector] Implement Shorlander's visual design (dark and light theme for: markupview, layoutview, computedview, ruleview)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Paul Rouget [:paul]]

The computed view interface is noisy, a bit messy and the code mixes XUL and HTML.

In this bug I will:

- remove references to XUL content
- implement a more responsive design
- optimize the layout
- introduce the black theme (with a switch mechanism)
- kill some UI elements (the MDN link and the footer)

Comment 1

[Joe Walker [:jwalker]]

I'm not sure I'm with you on removing the MDN link, maybe find another way, but I think that's really useful.

Also did you mean to make those bugs depend on this rather than the other way around?

Comment 2

[Paul Rouget [:paul]]

(In reply to Joe Walker [:jwalker] from comment #1)
> I'm not sure I'm with you on removing the MDN link, maybe find another way,
> but I think that's really useful.

The problem with this MDN link is that it's rarely useful, but it's always here, and it makes the UI noisy. I talked to Mike, and we thought that moving this link into the context menu, and keep the F1 keybinding should be enough.

> Also did you mean to make those bugs depend on this rather than the other
> way around?

I don't know.

Comment 3

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #2)
> (In reply to Joe Walker [:jwalker] from comment #1)
> > I'm not sure I'm with you on removing the MDN link, maybe find another way,
> > but I think that's really useful.
> 
> The problem with this MDN link is that it's rarely useful, but it's always
> here, and it makes the UI noisy. I talked to Mike, and we thought that
> moving this link into the context menu, and keep the F1 keybinding should be
> enough.

OK, makes sense.

Comment 4

[Paul Rouget [:paul]]

Attachment: PatchA v1: Computed View and new styling

Comment 5

[Paul Rouget [:paul]]

Attachment: PatchB v1: update markup view colors

Comment 6

[Paul Rouget [:paul]]

Attachment: PatchC v1: theme switching mechanism + floating scrollbars

Comment 7

[Paul Rouget [:paul]]

Attachment: PatchD v1: layout view colors update

Comment 8

[Paul Rouget [:paul]]

Rule View is missing. I haven't tested on Windows yet.

Comment 9

[Paul Rouget [:paul]]

PatchA is the biggest patch:

- introduces a devtools.theme pref
- rename csshtmltree files to computeview
- change the UI of the computed view:
  - XUL -> HTML
  - scale and overflow better
  - no MDN link visible (but F1 still works)
  - remove the footer with hints, we use the tooltip to say if things are matched/bestMatched/parentMatch.
- introduce generic theme CSS files

PatchB uses the generic theme CSS files in the markup view

PatchC introduces a mechanism to select the theme (light or dark, controlled by a pref for the moment)

PatchD uses the generic theme CSS files in the layout view

Comment 10

[Paul Rouget [:paul]]

Next steps:
- attach ruleview code (WIP)
- make sure I don't break any test
- write test for theme switching
- get a ui review (I'll provide builds)
- figure out how to expose theme switching to the user (command?)

Comment 11

[Paul Rouget [:paul]]

Attachment: screenshots

Not asking for a formal review for now. I'll provide builds to let you play with this once the ruleview is ready.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Paul Rouget [:paul] from comment #11)
> Created attachment 719063 [details]
> screenshots
> 
> Not asking for a formal review for now. I'll provide builds to let you play
> with this once the ruleview is ready.

This looks really good. Feedback from me:

- I believe we should not use monospace for the "browser styles" checkbox - that's UI and it shouldn't be monospaced (nor search).

- In terms of UI consistency, putting the search field at the bottom is surprising for me.

- each property value has an arrow prefix, that should be ":" or maybe nothing? The ">" is confusing (">" is used for selectors).

Comment 13

[Paul Rouget [:paul]]

Next steps:
- attach ruleview code (WIP)
- make sure I don't break any test
- write test for theme switching
- get a ui review (I'll provide builds)
- figure out how to expose theme switching to the user (command?)
- add license block to new files
- fix outlines around twisties

Comment 14

[Paul Rouget [:paul]]

Attachment: PatchE v1: ruleview update

Comment 15

[Paul Rouget [:paul]]

Next steps:
- make sure I don't break any test
- write test for theme switching
- get a ui review (I'll provide builds)
- figure out how to expose theme switching to the user (command?)
- add license block to new files
- fix outlines around twisties
- get a better design for the checkboxes
- "Rule" appears as tooltip in the ruleview

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 721726 [details] [diff] [review]
PatchE v1: ruleview update

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

Lookin great!

Comment 17

[Paul Rouget [:paul]]

Attachment: PatchF v1: font view update

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 723703 [details] [diff] [review]
PatchF v1: font view update

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

Looks great, r+

Comment 19

[sys.sgx]

Paul, the screenshots are amazing!
Just some feedback I'd like to add:

- The user should be able to switch theme through an "Options" panel, that should be visible at will. At some point you will need an Options panel in order to add more stuff, instead of going through the about:config way. (btw, what's that button on the upper left corner?..)

- Are the scrollbars mac-specific, or are they devtools specific?

Let us know for any changes. :)

Comment 20

[Paul Rouget [:paul]]

(In reply to sys.sgx from comment #19)
> Paul, the screenshots are amazing!
> Just some feedback I'd like to add:
> 
> - The user should be able to switch theme through an "Options" panel, that
> should be visible at will. At some point you will need an Options panel in
> order to add more stuff, instead of going through the about:config way.
> (btw, what's that button on the upper left corner?..)

Planned.

> - Are the scrollbars mac-specific, or are they devtools specific?

devtools specific.

Comment 21

[sys.sgx]

(In reply to Paul Rouget [:paul] from comment #20)
> (In reply to sys.sgx from comment #19)
> > Paul, the screenshots are amazing!
> > Just some feedback I'd like to add:
> > 
> > - The user should be able to switch theme through an "Options" panel, that
> > should be visible at will. At some point you will need an Options panel in
> > order to add more stuff, instead of going through the about:config way.
> > (btw, what's that button on the upper left corner?..)
> 
> Planned.
> 
> > - Are the scrollbars mac-specific, or are they devtools specific?
> 
> devtools specific.

Great for both! I guess we're gonna need to have the same theme on the JSTerminal as well.

Notice: The dark theme's scrollbars are a bit too thin, might need some more pixels in width for easier scrolling.

Comment 22

[Paul Rouget [:paul]]

Next steps:
- see comment 15
- make sure the responsive mode still works (we use floating scrollbars)

Comment 23

[Paul Rouget [:paul]]

Updated patches coming.

Next steps:
- make sure I don't break any test
- write test for theme switching

Will happen in follow-ups:
- get a ui review (I'll provide builds)
- get a better design for the checkboxes

Comment 24

[Paul Rouget [:paul]]

Attachment: PatchA v1.1: Computed View and new styling

Comment 25

[Paul Rouget [:paul]]

Attachment: PatchB v1.1: update markup view colors

Comment 26

[Paul Rouget [:paul]]

Attachment: PatchC v1.1: theme switching mechanism + floating scrollbars

Comment 27

[Paul Rouget [:paul]]

Attachment: PatchD v1.1: layout view colors update

Comment 28

[Paul Rouget [:paul]]

Attachment: PatchE v1.1: ruleview update

Comment 29

[Paul Rouget [:paul]]

Attachment: PatchF v1.1: font view update

Comment 30

[Paul Rouget [:paul]]

Attachment: PatchG v1.1: fix test failures

Comment 31

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=cb52e6268e74

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 727122 [details] [diff] [review]
PatchD v1.1: layout view colors update

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

::: browser/themes/osx/devtools/layoutview.css
@@ +15,5 @@
>    -moz-box-sizing: border-box;
>  }
>  
>  #element-size {
> +  /*color: hsl(210,100%,95%);*/

nit: was this left commented out on purpose? The same for Windows & Linux.

Comment 33

[Paul Rouget [:paul]]

Fixing more tests: https://tbpl.mozilla.org/?tree=Try&rev=77bf615a7bc7

Comment 34

[Paul Rouget [:paul]]

Attachment: all patches in one

Comment 35

[sys.sgx]

@Paul: plz point to a temp build for testing the release.

Comment 36

[Paul Rouget [:paul]]

This should fix most of the test failures: https://tbpl.mozilla.org/?tree=Try&rev=0a5035d1c235

Comment 37

[Girish Sharma [:Optimizer]]

Correct me if I am wrong, but in the patch floating-scrollbars.css file is importing itself on windows and linux due to which floating scrollbars in responsive mode are not working anymore.

I think you wanted to add the @import line in floating-scrollbars-light.css on windows and linux instead.

Comment 38

[Paul Rouget [:paul]]

Todo: Comment 23 +
- dark selection is low contrast
- we can't select the url in the font inspector

Comment 39

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=0a5c5d9e7361

Comment 40

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Still broken.

Comment 41

[Paul Rouget [:paul]]

Green at home: https://tbpl.mozilla.org/?tree=Try&rev=3cbb9fc5b240

Once this turns green, I'll file follow-ups for:
- get a ui review (I'll provide builds)
- get a better design for the checkboxes
- dark selection is low contrast
- we can't select the url in the font inspector

Comment 42

[Paul Rouget [:paul]]

Attachment: all patches in one

Comment 43

[Paul Rouget [:paul]]

Attachment: all patches in one (with tests)

Fixed the tests.

Comment 44

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 728256 [details] [diff] [review]
all patches in one (with tests)

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

r+ assuming try == green

Comment 45

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=24bf55426107

Comment 46

[Paul Rouget [:paul]]

Attachment: tmp

Comment 47

[Paul Rouget [:paul]]

Attachment: Remove context menus

Atomic approach: I can't make context menus work properly. Our copy/pasting mechanism is not really reliable (does it even work on Windows 8?), it's a lot of code, and it's a nightmare to test properly, and they don't bring much value. So I'm killing them.

Ctrl-c and Copy Selection are important though. This still works, but we need to have a proper context menu. As we use HTML documents, I think we should use a contentArea context menu, and enhance it with <html:menu> tags. I'll file a bug about that if Mike is ok with this patch.

Comment 48

[Paul Rouget [:paul]]

Attachment: all patches in one (with tests + no ctx menus)

Comment 49

[Paul Rouget [:paul]]

try number 156: https://tbpl.mozilla.org/?tree=Try&rev=e6e3afb7a926

Comment 50

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/36332ce24dd1

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36332ce24dd1