change color theme of box-model view

NEW
Unassigned

Status

()

Firefox
Developer Tools: Computed Styles Inspector
P2
normal
a year ago
23 days ago

People

(Reporter: gasolin, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-needed, feature})

51 Branch
Firefox 51
dev-doc-needed, feature
Points:
---

Firefox Tracking Flags

(firefox51 affected, firefox52 wontfix, firefox53 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments, 7 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8777260 [details]
box-model view color spec

from UI spec in bug 1150496 , we'd like have a nice looking box-model view
Component: Developer Tools: Inspector → Developer Tools: Computed Styles Inspector
Priority: -- → P2
(Reporter)

Comment 1

a year ago
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?
Flags: needinfo?(hholmes)
(Reporter)

Comment 2

a year ago
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
Created attachment 8777684 [details]
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.
Created attachment 8777686 [details] [diff] [review]
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.
(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?
Flags: needinfo?(hholmes) → needinfo?(odvarko)
(Reporter)

Comment 6

a year ago
Created attachment 8778108 [details]
after patch

screenshot after patch
Attachment #8777678 - Attachment is obsolete: true
(Reporter)

Comment 7

a year ago
Created attachment 8778112 [details]
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/
(Reporter)

Comment 8

a year ago
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.
Attachment #8778112 - Flags: feedback?(gl)
(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?
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.
(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
Flags: needinfo?(odvarko)
(Reporter)

Comment 12

a year ago
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/
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.
Flags: needinfo?(gasolin)
Attachment #8778112 - Flags: feedback?(gl)
(Reporter)

Comment 14

a year ago
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.
Flags: needinfo?(gasolin)
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
(Reporter)

Comment 16

a year ago
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
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 hidden (mozreview-request)
(Reporter)

Comment 19

a year ago
Created attachment 8781849 [details]
light theme

here's the screenshot of light theme after patch
Attachment #8778108 - Attachment is obsolete: true
(Reporter)

Comment 20

a year ago
Created attachment 8781850 [details]
dark theme

here's the screenshot of dark theme after patch
(Reporter)

Comment 21

a year ago
Created attachment 8781852 [details]
firebug theme

here's the screenshot of firebug theme after patch
(Reporter)

Comment 22

a year ago
thanks for point out the highlighter.css , the new PR also covered that part!
Sorry, will get to this in the morning
(Reporter)

Comment 24

a year ago
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?
Flags: needinfo?(hholmes)
(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 :)
Flags: needinfo?(hholmes)

Comment 26

a year ago
(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 hidden (mozreview-request)
(Reporter)

Comment 28

a year ago
Created attachment 8783415 [details]
dark theme

Good catch Tim! I've update the PR and will update dark & firebug screenshot after patch
Attachment #8781850 - Attachment is obsolete: true
(Reporter)

Comment 29

a year ago
Created attachment 8783416 [details]
firebug theme
Attachment #8781852 - Attachment is obsolete: true

Comment 30

a year ago
mozreview-review
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
Attachment #8778112 - Flags: review?(gl)
Comment hidden (mozreview-request)
(Reporter)

Comment 32

a year ago
mozreview-review-reply
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!
(Reporter)

Comment 33

a year ago
Created attachment 8785118 [details]
auto label position

the patch also fixed auto label position

Comment 34

a year ago
mozreview-review
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
Attachment #8778112 - Flags: review?(gl)
(Reporter)

Comment 35

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8778112 - Flags: ui-review?
(Reporter)

Comment 38

a year ago
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.
Attachment #8778112 - Flags: ui-review? → ui-review?(hholmes)
(Reporter)

Comment 39

a year ago
Created attachment 8787032 [details]
light theme
Attachment #8781849 - Attachment is obsolete: true
(Reporter)

Comment 40

a year ago
Created attachment 8787033 [details]
dark theme
Attachment #8783415 - Attachment is obsolete: true
(Reporter)

Comment 41

a year ago
Created attachment 8787034 [details]
firebug theme
Attachment #8783416 - Attachment is obsolete: true
(Reporter)

Comment 42

a year ago
Created attachment 8787035 [details]
new theme compare with v48
Attachment #8778112 - Flags: ui-review?(hholmes) → ui-review+

Comment 43

a year ago
mozreview-review
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.
Attachment #8778112 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 45

a year ago
nits addressed, thanks!
Keywords: checkin-needed, feature
Version: Trunk → 51 Branch

Comment 46

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/4329f53c9f9f
Change color theme of box-model view. r=gl
Keywords: checkin-needed

Comment 47

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4329f53c9f9f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
The color changes require some DevTools documentation pages to be updated.

Sebastian
Keywords: dev-doc-needed
Fred, is this something manual QA could help test?
Flags: qe-verify?
Flags: needinfo?(gasolin)
(Reporter)

Comment 50

a year ago
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
Flags: needinfo?(gasolin)
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.
See bug 1301676 about comment 51.
(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.
Flags: qe-verify? → qe-verify+
QA Contact: cristian.comorasu
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!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
https://hg.mozilla.org/integration/fx-team/rev/0800dc3f7d13725512ec4a0ad87066f9efea2e2c
Backed out changeset 4329f53c9f9f Bug 1291638 r=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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
(Reporter)

Comment 58

a year ago
Got it, thanks for notice
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!
Flags: needinfo?(gasolin)
(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
Flags: needinfo?(gasolin)
(Reporter)

Comment 61

a year ago
Yap, let's ride the next train
status-firefox51: verified → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 62

9 months ago
Gabriel, since we have UX again (\o/), do you think its the right time to put this in stack?
Flags: needinfo?(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.
Flags: needinfo?(gl)
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix

Comment 65

2 months ago
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 ?
Status: REOPENED → ASSIGNED
Flags: needinfo?(gl)
We still can't move forward with because the color involved in this patch because of Bug 1301676.
Flags: needinfo?(gl)
(Reporter)

Updated

23 days ago
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.