Closed Bug 836233 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(3 files, 17 obsolete files)

1.30 MB, image/png
Details
53.74 KB, patch
miker
: review+
Details | Diff | Splinter Review
196.78 KB, patch
Details | Diff | Splinter Review
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)
Blocks: 835805
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?
(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.
(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.
Summary: [Computed view] rething the UI → [Computed view] rethink the UI
No longer blocks: 826685
Depends on: 834336, 826685
Depends on: 834187
Blocks: 840261
Blocks: 802837
Assignee: nobody → paul
Depends on: 480356
Attached file PatchB v1: update markup view colors (obsolete) —
Attached file PatchD v1: layout view colors update (obsolete) —
Summary: [Computed view] rethink the UI → [Inspector] Implement Shorlander's visual design (dark and light theme for: markupview, layoutview, computedview, ruleview)
Rule View is missing. I haven't tested on Windows yet.
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
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?)
Attachment #719055 - Flags: feedback?(mratcliffe)
Attachment #719057 - Flags: feedback?(mratcliffe)
Attachment #719058 - Flags: feedback?(mratcliffe)
Attachment #719059 - Flags: feedback?(mratcliffe)
Attached image screenshots
Not asking for a formal review for now. I'll provide builds to let you play with this once the ruleview is ready.
Attachment #719063 - Flags: feedback?(shorlander)
(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).
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
Attachment #719055 - Flags: feedback?(mratcliffe) → feedback+
Attachment #719057 - Flags: feedback?(mratcliffe) → feedback+
Attachment #719058 - Flags: feedback?(mratcliffe) → feedback+
Attachment #719059 - Flags: feedback?(mratcliffe) → feedback+
Attached patch PatchE v1: ruleview update (obsolete) — Splinter Review
Attachment #721726 - Flags: feedback?(mratcliffe)
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 on attachment 721726 [details] [diff] [review]
PatchE v1: ruleview update

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

Lookin great!
Attachment #721726 - Flags: feedback?(mratcliffe) → feedback+
Attached patch PatchF v1: font view update (obsolete) — Splinter Review
Attachment #723703 - Flags: feedback?(mratcliffe)
Comment on attachment 723703 [details] [diff] [review]
PatchF v1: font view update

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

Looks great, r+
Attachment #723703 - Flags: feedback?(mratcliffe) → feedback+
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. :)
(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.
(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.
Next steps:
- see comment 15
- make sure the responsive mode still works (we use floating scrollbars)
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
Attachment #719055 - Attachment is obsolete: true
Attachment #727119 - Flags: review?(mratcliffe)
Attachment #719057 - Attachment is obsolete: true
Attachment #727120 - Flags: review?(mratcliffe)
Attachment #719058 - Attachment is obsolete: true
Attachment #727121 - Flags: review?(mratcliffe)
Attachment #719059 - Attachment is obsolete: true
Attachment #727122 - Flags: review?(mratcliffe)
Attached patch PatchE v1.1: ruleview update (obsolete) — Splinter Review
Attachment #721726 - Attachment is obsolete: true
Attachment #727123 - Flags: review?(mratcliffe)
Attached patch PatchF v1.1: font view update (obsolete) — Splinter Review
Attachment #723703 - Attachment is obsolete: true
Attachment #727124 - Flags: review?(mratcliffe)
Attached patch PatchG v1.1: fix test failures (obsolete) — Splinter Review
Attachment #727127 - Flags: review?(mratcliffe)
Attachment #727119 - Flags: review?(mratcliffe) → review+
Attachment #727120 - Flags: review?(mratcliffe) → review+
Attachment #727121 - Flags: review?(mratcliffe) → review+
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.
Attachment #727122 - Flags: review?(mratcliffe) → review+
Attachment #727123 - Flags: review?(mratcliffe) → review+
Attachment #727124 - Flags: review?(mratcliffe) → review+
Attachment #727127 - Flags: review?(mratcliffe) → review+
Attached patch all patches in one (obsolete) — Splinter Review
@Paul: plz point to a temp build for testing the release.
This should fix most of the test failures: https://tbpl.mozilla.org/?tree=Try&rev=0a5035d1c235
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.
Todo: Comment 23 +
- dark selection is low contrast
- we can't select the url in the font inspector
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
Attached patch all patches in one (obsolete) — Splinter Review
Attachment #727119 - Attachment is obsolete: true
Attachment #727120 - Attachment is obsolete: true
Attachment #727121 - Attachment is obsolete: true
Attachment #727122 - Attachment is obsolete: true
Attachment #727123 - Attachment is obsolete: true
Attachment #727124 - Attachment is obsolete: true
Attachment #727127 - Attachment is obsolete: true
Attachment #727185 - Attachment is obsolete: true
Attached patch all patches in one (with tests) (obsolete) — Splinter Review
Fixed the tests.
Attachment #728249 - Attachment is obsolete: true
Attachment #728256 - Flags: review?(mratcliffe)
Comment on attachment 728256 [details] [diff] [review]
all patches in one (with tests)

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

r+ assuming try == green
Attachment #728256 - Flags: review?(mratcliffe) → review+
Attached patch tmp (obsolete) — Splinter Review
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.
Attachment #729660 - Attachment is obsolete: true
Attachment #730181 - Flags: review?(mratcliffe)
Attachment #728256 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #730181 - Flags: review?(mratcliffe) → review+
Blocks: 855499
Blocks: 855502
Blocks: 855504
Depends on: 855518
Depends on: 855520
Depends on: 855523
https://hg.mozilla.org/mozilla-central/rev/36332ce24dd1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Depends on: 858493
Depends on: 859686
Duplicate of this bug: 840261
Attachment #719063 - Flags: feedback?(shorlander)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.