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

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 22
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

1.30 MB, image/png
Details
53.74 KB, patch
miker
: review+
Details | Diff | Splinter Review
196.78 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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)
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 831711
(Assignee)

Comment 2

5 years ago
(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
(Assignee)

Updated

5 years ago
No longer blocks: 826685
Depends on: 834336, 826685
(Assignee)

Updated

5 years ago
Depends on: 834187
(Assignee)

Updated

5 years ago
Blocks: 840261
(Assignee)

Updated

5 years ago
Blocks: 802837
Assignee: nobody → paul
(Assignee)

Updated

5 years ago
Depends on: 480356
(Assignee)

Comment 4

5 years ago
Created attachment 719055 [details] [diff] [review]
PatchA v1: Computed View and new styling
(Assignee)

Comment 5

5 years ago
Created attachment 719057 [details]
PatchB v1: update markup view colors
(Assignee)

Comment 6

5 years ago
Created attachment 719058 [details]
PatchC v1: theme switching mechanism + floating scrollbars
(Assignee)

Comment 7

5 years ago
Created attachment 719059 [details]
PatchD v1: layout view colors update
(Assignee)

Updated

5 years ago
Summary: [Computed view] rethink the UI → [Inspector] Implement Shorlander's visual design (dark and light theme for: markupview, layoutview, computedview, ruleview)
(Assignee)

Comment 8

5 years ago
Rule View is missing. I haven't tested on Windows yet.
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
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?)
(Assignee)

Updated

5 years ago
Attachment #719055 - Flags: feedback?(mratcliffe)
(Assignee)

Updated

5 years ago
Attachment #719057 - Flags: feedback?(mratcliffe)
(Assignee)

Updated

5 years ago
Attachment #719058 - Flags: feedback?(mratcliffe)
(Assignee)

Updated

5 years ago
Attachment #719059 - Flags: feedback?(mratcliffe)
(Assignee)

Comment 11

5 years ago
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.
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).
(Assignee)

Comment 13

5 years ago
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+
(Assignee)

Comment 14

5 years ago
Created attachment 721726 [details] [diff] [review]
PatchE v1: ruleview update
Attachment #721726 - Flags: feedback?(mratcliffe)
(Assignee)

Comment 15

5 years ago
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+
(Assignee)

Comment 17

5 years ago
Created attachment 723703 [details] [diff] [review]
PatchF v1: font view update
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+

Comment 19

5 years ago
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. :)
(Assignee)

Comment 20

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 22

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

Comment 23

5 years ago
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
(Assignee)

Comment 24

5 years ago
Created attachment 727119 [details] [diff] [review]
PatchA v1.1: Computed View and new styling
Attachment #719055 - Attachment is obsolete: true
Attachment #727119 - Flags: review?(mratcliffe)
(Assignee)

Comment 25

5 years ago
Created attachment 727120 [details] [diff] [review]
PatchB v1.1: update markup view colors
Attachment #719057 - Attachment is obsolete: true
Attachment #727120 - Flags: review?(mratcliffe)
(Assignee)

Comment 26

5 years ago
Created attachment 727121 [details] [diff] [review]
PatchC v1.1: theme switching mechanism + floating scrollbars
Attachment #719058 - Attachment is obsolete: true
Attachment #727121 - Flags: review?(mratcliffe)
(Assignee)

Comment 27

5 years ago
Created attachment 727122 [details] [diff] [review]
PatchD v1.1: layout view colors update
Attachment #719059 - Attachment is obsolete: true
Attachment #727122 - Flags: review?(mratcliffe)
(Assignee)

Comment 28

5 years ago
Created attachment 727123 [details] [diff] [review]
PatchE v1.1: ruleview update
Attachment #721726 - Attachment is obsolete: true
Attachment #727123 - Flags: review?(mratcliffe)
(Assignee)

Comment 29

5 years ago
Created attachment 727124 [details] [diff] [review]
PatchF v1.1: font view update
Attachment #723703 - Attachment is obsolete: true
Attachment #727124 - Flags: review?(mratcliffe)
(Assignee)

Comment 30

5 years ago
Created attachment 727127 [details] [diff] [review]
PatchG v1.1: fix test failures
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+
(Assignee)

Comment 34

5 years ago
Created attachment 727185 [details] [diff] [review]
all patches in one

Comment 35

5 years ago
@Paul: plz point to a temp build for testing the release.
(Assignee)

Comment 36

5 years ago
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.
Blocks: 851546
(Assignee)

Comment 38

5 years ago
Todo: Comment 23 +
- dark selection is low contrast
- we can't select the url in the font inspector
(Assignee)

Comment 41

5 years ago
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
(Assignee)

Comment 42

5 years ago
Created attachment 728249 [details] [diff] [review]
all patches in one
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
(Assignee)

Comment 43

5 years ago
Created attachment 728256 [details] [diff] [review]
all patches in one (with tests)

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+
(Assignee)

Comment 47

5 years ago
Created attachment 730181 [details] [diff] [review]
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.
Attachment #729660 - Attachment is obsolete: true
Attachment #730181 - Flags: review?(mratcliffe)
(Assignee)

Comment 48

5 years ago
Created attachment 730182 [details] [diff] [review]
all patches in one (with tests + no ctx menus)
Attachment #728256 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #730181 - Flags: review?(mratcliffe) → review+
(Assignee)

Comment 50

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/36332ce24dd1
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 855499
(Assignee)

Updated

5 years ago
Blocks: 855502
(Assignee)

Updated

5 years ago
Blocks: 855504
Depends on: 855518
Depends on: 855520
Depends on: 855523
https://hg.mozilla.org/mozilla-central/rev/36332ce24dd1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Depends on: 858493

Updated

5 years ago
Depends on: 859686
Depends on: 866656
(Assignee)

Updated

5 years ago
Duplicate of this bug: 840261

Updated

4 years ago
Attachment #719063 - Flags: feedback?(shorlander)
You need to log in before you can comment on or make changes to this bug.