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

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?

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

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

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?

screenshot after patch

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

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.

(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

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.

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.

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

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

here's the screenshot of light theme after patch

here's the screenshot of dark theme after patch

here's the screenshot of firebug theme after patch

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

Sorry, will get to this in the morning

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?

(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 :)

(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.

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

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 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!

the patch also fixed auto label position

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 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 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 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.

nits addressed, thanks!

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

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

Fred, is this something manual QA could help test?

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

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.

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!

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.

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.

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!

(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

Yap, let's ride the next train

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

(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.

Mass wontfix for bugs affecting firefox 52.

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 ?

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