1291638 - change color theme of box-model view

Status: NEW

(DevTools :: Inspector, enhancement, P2)

Description

[[e-mail]]

Attachment: box-model view color spec

from UI spec in bug 1150496 , we'd like have a nice looking box-model view

Comment 1

[[e-mail]]

Attachment: text color not applied

Here's work in progress version that applied new color theme to the box model

Helen, can you help confirm the strings' color on the model view in black/light/firebug theme? (margin, border, padding ...etc), or just use the default color(black) as spec shows?

Comment 2

[[e-mail]]

FYR, if we took `--theme-body-color` CSS variable as main font color, 

in light theme it means #393f4c, 
in firebug theme it means #18191a;
in dark theme it means #8fa1b2

The font color in dark theme is too light, we can overwrite it with --theme-body-background (#393f4c) so the box model will looks like the same as in light theme

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: box-model-in-page-mockup.png

Not quite sure if this will help, but it looks like the colors for the legend is #1A1C22 in an older mockup.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1108325.patch

Just attaching an older patch I had made originally to change to the new colors. I don't think my approach is necessarily correct since you can probably just change the border-color and outline color. Whereas I changed the elements to make it work by using background-color and border-color. It looks like the white and dark theme are the same colors. We usually defer to Honza for firebug theming colors.

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Fred Lin [:gasolin] from comment #1)
> Created attachment 8777678 [details]
> text color not applied
> 
> Here's work in progress version that applied new color theme to the box model
> 
> Helen, can you help confirm the strings' color on the model view in
> black/light/firebug theme? (margin, border, padding ...etc), or just use the
> default color(black) as spec shows?

Hey Fred,

For both light and dark theme, the text color is #393F4C. (The box-model colors shouldn't need to change across themes; my thought process with this explanation: https://cl.ly/2c240V1B1Z1v is that the background color for the box model in both themes would be white, making the box model look like this across themes: https://cl.ly/0b3G3u320J2Y)

Honza, do you have thoughts on what this should look like in Firebug? Should we use this same style in Firebug, or keep Firebug the same?

Comment 6

[[e-mail]]

Attachment: after patch

screenshot after patch

Comment 7

[[e-mail]]

Attachment: Bug 1291638 - change color theme of box-model view;

Review commit: https://reviewboard.mozilla.org/r/69490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69490/

Comment 8

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Thanks for advices Gabriel! At the end I inherit current border + outline approach because the opacity is overlapped when I turned to background:rgba + border. The defect is we'd have to use non-standard -moz-outline-radius if we want to have round corner effect in box model.

The patch use #1A1C22 as text color until we get more feedback, and tuned the text position based on the fact that we add 2px border(outline) on our box model.

Comment 9

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Fred Lin [:gasolin] from comment #6)
> Created attachment 8778108 [details]
> after patch
> 
> screenshot after patch

Is it possible to increase the side of the box model around the labels? (margin, border, padding) They're looking a little tight right now. Original designs: https://cl.ly/1w0v1D2z3N2e

> Thanks for advices Gabriel! At the end I inherit current border + outline
> approach because the opacity is overlapped when I turned to background:rgba
> + border. The defect is we'd have to use non-standard -moz-outline-radius if
> we want to have round corner effect in box model.
Foiled again :( Will this be solved by devtools-html?

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Hi Fred,

I will get to this feedback in the morning. My current thoughts are we should implement this in a way where we no longer have any overlapping opacity. My original patch tries to accomplish this, but I will need to test out what is the best approach before providing more informative feedback. From a quick examination of your patch, I am a bit wary of the approach because of the need to adjust the top/left/right/bottom positions. I still require a closer look to see what is going.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #5)
> Honza, do you have thoughts on what this should look like in Firebug? Should
> we use this same style in Firebug
Yes, I think we should use the same style.

Honza

Comment 12

[[e-mail]]

Here are some jsfiddle to quicker test both approaches

from gl's original patch
https://jsfiddle.net/gasolin/xg1p3ckz/3/


from fred's patch
https://jsfiddle.net/gasolin/xg1p3ckz/4/

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

So, I figured out the overlapping issue with my original approach. We would have to use a hex value instead of rgba because providing a value that is <1 in the alpha value would make it less opague / more transparent. This would cause the opacity overlapping issue you have mention. 

I still think the background-color / border-color is the best approach. Please let me know what you think Fred given the usage of non-rgba values with this approach.

Comment 14

[[e-mail]]

Though MDN said its better to use rgba https://developer.mozilla.org/en-US/docs/Web/CSS/opacity

If the overlapping opacity issue can be solved, we have no reason to use border + outline hack.

Comment 15

[Helen V. Holmes (:helenvholmes) (:✨)]

Updated the Invision link to have the hex values of the boxes without opacity values. You can find those under the "Themes" section, in "bg sans opacity": https://cl.ly/0X1U2D3r302f

Comment 16

[[e-mail]]

Thanks helen's update, here's how it looks now (with background-color/border-color) 
https://jsfiddle.net/gasolin/xg1p3ckz/5/

I'll use this as a new base

Comment 17

[Patrick Brosset <:pbro>]

Looking at the patches here, I don't see the highlighter colors being changed. We should really change both the box-model *and* the highlighter at the same time. They both represent the same information about an element and therefore should share the same colors.
The highlighter's colors are defined in http://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters.css

Comment 18

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/1-2/

Comment 19

[[e-mail]]

Attachment: light theme

here's the screenshot of light theme after patch

Comment 20

[[e-mail]]

Attachment: dark theme

here's the screenshot of dark theme after patch

Comment 21

[[e-mail]]

Attachment: firebug theme

here's the screenshot of firebug theme after patch

Comment 22

[[e-mail]]

thanks for point out the highlighter.css , the new PR also covered that part!

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Sorry, will get to this in the morning

Comment 24

[[e-mail]]

Attachment: highlighter

Helen, original highlighter (the box color shown in target web page) does not contain border color. 

Current patch does not add border color for highlighter, should I add the border color on it?

Comment 25

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Fred Lin [:gasolin] from comment #24)
> Created attachment 8782378 [details]
> highlighter
> 
> Helen, original highlighter (the box color shown in target web page) does
> not contain border color. 
> 
> Current patch does not add border color for highlighter, should I add the
> border color on it?

Patrick and I chatted about this, and decided that a border would obscure the web content in a way we didn't want. So no need :)

Comment 26

[Tim Nguyen :ntim]

(In reply to Fred Lin [:gasolin] from comment #20)
> Created attachment 8781850 [details]
> dark theme
> 
> here's the screenshot of dark theme after patch

Seems like there's an issue with white artifacts on all 4 corners in your screenshots. There's probably some border-radius not being clipped correctly or some element behind having a white background.

Comment 27

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/2-3/

Comment 28

[[e-mail]]

Attachment: dark theme

Good catch Tim! I've update the PR and will update dark & firebug screenshot after patch

Comment 29

[[e-mail]]

Attachment: firebug theme

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

https://reviewboard.mozilla.org/r/69490/#review71602

The changes look good, but I think we need to fix the positioning of the editable fields since we switched to using background and border.

::: devtools/client/themes/layout.css:60
(Diff revision 3)
> -  border-color: hsla(210,100%,85%,0.2);
> -  border-width: 18px;
>    border-style: solid;
> -  outline: dotted 1px hsl(210,100%,85%);
> -}
> -
> +  border-width: 2px;
> +  padding: 16px;
> +  border-radius: 2px;

Group the border- styles together (move this line up)

::: devtools/client/themes/layout.css:66
(Diff revision 3)
>  }
>  
>  /* Regions colors */
>  
>  #layout-margins {
> -  border-color: #edff64;
> +  background-color: #91bce1;

This should be #9ebce1 I believe.

::: devtools/client/themes/layout.css:67
(Diff revision 3)
>  
>  /* Regions colors */
>  
>  #layout-margins {
> -  border-color: #edff64;
> +  background-color: #91bce1;
> +  border-color: #2f5b81;

This should also be #2f5b8e

Comment 31

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/3-4/

Comment 32

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

https://reviewboard.mozilla.org/r/69490/#review71602

issue addressed,  fix the positioning of the editable field and the auto label

> This should be #9ebce1 I believe.

you are absolutely right!

Comment 33

[[e-mail]]

Attachment: auto label position

the patch also fixed auto label position

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

https://reviewboard.mozilla.org/r/69490/#review73308

::: devtools/client/themes/layout.css:42
(Diff revisions 3 - 4)
>  #layout-main {
>    position: relative;
>    box-sizing: border-box;
> +  /* The regions are semi-transparent, so the white background is partly
> +     visible */
> +  background-color: white;

I see a white edge in the dark theme as a result of the border-radius. I would probably remove the background-color entirely or go with background-color: transparent.

::: devtools/client/themes/layout.css:51
(Diff revision 4)
> -  color: var(--theme-highlight-blue);
> -}
> -
>  /* Regions are 3 nested elements with wide borders and outlines */
>  
>  #layout-content {

We need to still align (vertically center) the layout-content text as a result of the border radius. When we do align the layout context text we should also adjust the left/right editable fields for margin/border/padding.

::: devtools/client/themes/layout.css:85
(Diff revision 4)
> +  border-color: #00c79a;
>  }
>  
>  #layout-content {
> -  background-color: #87ceeb;
> +  background-color: #f0f7ff;
> +  border: solid 2px #eaf4ff;

I prefer "border: 2px solid #eaf4ff"

::: devtools/client/themes/layout.css:218
(Diff revision 4)
>  /* Legend: displayed inside regions */
>  
>  .layout-legend {
> +  color: #1A1C22;
>    position: absolute;
>    margin: 2px 6px;

The legends looks like they need a bit of a top margin. I found "margin: 3px 6px" to look quite nice, but this might be a Helen question. We should do an ui-review before landing.

::: devtools/server/actors/highlighters.css:83
(Diff revision 4)
>  :-moz-native-anonymous .box-model-border {
> -  fill: #444444;
> +  fill: #b3c8e2;
>  }
>  
>  :-moz-native-anonymous .box-model-margin {
> -  fill: #edff64;
> +  fill: #91bce1;

I think this is still wrong - it should be #9ebce1 according to the spec https://projects.invisionapp.com/share/W287NPLAQ#/screens/179720590_Better_Box_Model

Comment 35

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

https://reviewboard.mozilla.org/r/69490/#review73308

> I see a white edge in the dark theme as a result of the border-radius. I would probably remove the background-color entirely or go with background-color: transparent.

Thanks for catch that. Remove the background-color entirely or set it transparent  still cause  a white edge  in firebug theme. I'd set `background-color` to the same `--theme-body-color`  for `.theme-firebug #layout-main`

> We need to still align (vertically center) the layout-content text as a result of the border radius. When we do align the layout context text we should also adjust the left/right editable fields for margin/border/padding.

Do you mean we should apply flex layout like this for all editable fields?

Comment 36

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/4-5/

Comment 37

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/5-6/

Comment 38

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Helen, I'll attach box-model for all themes, please take a look and make sure if the result is as expected.

Comment 39

[[e-mail]]

Attachment: light theme

Comment 40

[[e-mail]]

Attachment: dark theme

Comment 41

[[e-mail]]

Attachment: firebug theme

Comment 42

[[e-mail]]

Attachment: new theme compare with v48

Comment 43

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

https://reviewboard.mozilla.org/r/69490/#review75344

SHIP IT.

::: devtools/client/themes/layout.css:50
(Diff revision 6)
> -
>  /* Regions are 3 nested elements with wide borders and outlines */
>  
>  #layout-content {
>    height: 18px;
> +  background-color: white;

We don't need this "background-color: white". It is actually overwritten by line 81.

::: devtools/client/themes/layout.css:51
(Diff revision 6)
>  /* Regions are 3 nested elements with wide borders and outlines */
>  
>  #layout-content {
>    height: 18px;
> +  background-color: white;
> +  border-radius: 2px;

Don't need this border-radius as well since we set the border property in line 82.

Comment 44

[[e-mail]]

Comment on attachment 8778112 [details]
Bug 1291638 - change color theme of box-model view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69490/diff/6-7/

Comment 45

[[e-mail]]

nits addressed, thanks!

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4329f53c9f9f
Change color theme of box-model view. r=gl

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4329f53c9f9f

Comment 48

[Sebastian Zartner [:sebo]]

The color changes require some DevTools documentation pages to be updated.

Sebastian

Comment 49

[Andrei Vaida [:avaida]]

Fred, is this something manual QA could help test?

Comment 50

[[e-mail]]

I guess not much, what need to verify might be:

1. Open devtools > inspector and check the box-model visually correct
2. Change different theme via `Toolbox Options` and check the box-model visually correct
3. Use `pick an element` tool at top left corner and check if select element has same color theme as box-model

Comment 51

[Patrick Brosset <:pbro>]

I'm not sure what's the relation between this bug and bug 1108325, but I think that other one should probably be closed, because the work was done here.

However, in that other bug, I did make a comment that I think is important we look into:

Bug 1108325 comment 37, attachment 8760655 [details]:
> Here's what the highlighter would look like on an element that has only a
> content area, over a #eeeeee background.

Basically, the content area color is so light that on some backgrounds, it doesn't even show.
Maybe having the guides around it is enough of a clue, but maybe we want to address this.

In fact this is a general comment: depending on the background of the page you are inspecting, some of the margins or paddings or borders or content area of the highlighter might be really hard to see.
I wonder if we could adapt the colors depending on what is currently being highlighted.

I'll spend some time and make a test page that shows this.

Comment 52

[Patrick Brosset <:pbro>]

See bug 1301676 about comment 51.

Comment 53

[Andrei Vaida [:avaida]]

(In reply to Fred Lin [:gasolin] from comment #50)
> I guess not much, what need to verify might be:
> 
> 1. Open devtools > inspector and check the box-model visually correct
> 2. Change different theme via `Toolbox Options` and check the box-model
> visually correct
> 3. Use `pick an element` tool at top left corner and check if select element
> has same color theme as box-model

Thank you for following up on this, Fred! Cristian is going to handle this feature in terms of QA.

Comment 54

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I verified the background colors and the margins of the box model view using Fx 51.0a1, build ID: 20160911030419 on the following platforms: Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.

The colors were changed accordingly.

Cheers!

Comment 55

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/0800dc3f7d13725512ec4a0ad87066f9efea2e2c
Backed out changeset 4329f53c9f9f Bug 1291638 r=gl

Comment 56

[Gabriel Luong [:gl] (ΦωΦ)]

Backed out because of Bug 1301676, which will address making sure all of the highlighter's regions are always visible since the colors are fairly similar.

Comment 57

[Gabriel Luong [:gl] (ΦωΦ)]

Sorry Fred, we had to backout your patch for this release to address some ux issues about the colors being too similar for the margin and border. I believe this will mostly be addressed in Bug 1301676. I don't think you will need to do anything regarding the current patch.

Comment 58

[[e-mail]]

Got it, thanks for notice

Comment 59

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Hello!

Could you please tell me if this feature will be uplifted in Fx 51.0a2 after it will be fixed or it will remain on nightly?

Thank you!

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Cristian Comorasu from comment #59)
> Hello!
> 
> Could you please tell me if this feature will be uplifted in Fx 51.0a2 after
> it will be fixed or it will remain on nightly?
> 
> Thank you!

It will probably remain on nightly and ride the now train cycle

Comment 61

[[e-mail]]

Yap, let's ride the next train

Comment 62

[[e-mail]]

Gabriel, since we have UX again (\o/), do you think its the right time to put this in stack?

Comment 63

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Fred Lin [:gasolin] from comment #62)
> Gabriel, since we have UX again (\o/), do you think its the right time to
> put this in stack?

Hi Fred, we still don't actually have a new UX yet. The problem with moving forward with this patch at this time is that the colors are simply too bright, and the colors for border and margin are also quite similar.

Comment 64

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 65

[Franck (Wip)]

Hello!
With the latest refresh of the Dark theme following the new photon style, the box model now really stands out.
Can't we evaluate again to land this patch ?

Comment 66

[Gabriel Luong [:gl] (ΦωΦ)]

We still can't move forward with because the color involved in this patch because of Bug 1301676.