1436549 - Implement the photon redesign of the box model

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

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

Attachment: New Box Model Design

This is to implement the photon redesign of the box model. The end goal is to restyle the existing box model so that it looks like the bottom box model in the attached screenshot.

Comment 1

[Victoria Wang [:victoria]]

Here's a new and improved version, with slightly different colors and a new dashed border! You can disregard the previous image. 

https://mozilla.invisionapp.com/share/Z3F7OGCTK#/278078739_newboxmodel_With_Colors

Feel free to ping me if you have questions!

Comment 2

[Lawson C]

Attachment: cssbox

Incomplete patch to get some feedback

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8949935 [details] [diff] [review]
cssbox

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

I'm stealing this review from Gabriel who seems to have quite a lot of them.
Gabriel, feel free to re-review if you want.
I've tested this, it looks nice, like the mockup. I've also made some comments on the code. See below. I think we need some clean up of the CSS mostly.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +465,5 @@
> +      dom.span(
> +        {
> +          className: "boxmodel-legend",
> +          "data-box": "position",
> +          title: "position",

This should be a localized string instead of being hard-coded in English.
If you look at the other boxmodel-legend elements, you'll see that they use things like that:
BOXMODEL_L10N.getStr("boxmodel.margin")
So you should also do this for position:
BOXMODEL_L10N.getStr("boxmodel.position")
which means you'll also need to add the string in the localization file: devtools\client\locales\en-US\boxmodel.properties

::: devtools/client/themes/boxmodel.css
@@ +54,3 @@
>  .boxmodel-margin,
> +.boxmodel-padding{
> +  color: #737373;

There are new hardcoded colors in this file. I'm a little uncomfortable with them because any hardcoded color means it's harder to theme devtools.
Now, it might be fine here because these are colors of the box-model diagram that are most likely not suppose to change when we change the devtools theme anyway.
But still, I'd like to make them to variables instead, this way they have names that are self explanatory, and we avoid repeating them over and over again.

Something like that:

.boxmodel-container {
  ...
  --content-border-color: #54A9FF;
  --margin-border-color: #fdffdf;
  ...
}

And so on.

This way, everywhere you need to use the #fdffdf color in the file, you can use var(--margin-border-color) instead. And it'll be very easy for someone else to change the colors later.

@@ +95,4 @@
>  /* Regions colors */
>  
>  .boxmodel-margins {
> +  border-color: #fdffdf;

No need to override the colors. You should just replace them instead.
If you look further up in the file, there is already a .boxmodel-margins { ... } rule, and it already defines a border-color, which is different from the one here. So, instead of overriding the color here, you should just define the color once in the previous .boxmodel-margins { ... } rule, and then delete this rule.

Same comment for the -borders, -paddings and -contents rules below.

@@ +332,5 @@
>    -moz-user-select: none;
>  }
>  
> +/*
> +.boxmodel-editable[data-box="border"] {

If this rule is commented out, then it isn't needed at all, and you should remove it altogether.

@@ +341,5 @@
> +}*/
> +
> +.boxmodel-editable[title="border-top-width"]{
> +  background-color: #737373;
> +  position: relative; 

nit: there is a trailing space at the end of this line, and also in other lines further below. Please remove trailing spaces. Our review tools show them as errors, and our linter too. I suggest configuring your text editor to auto-remove them on save. This way you don't have to think about it.

@@ +389,5 @@
> +}
> +.boxmodel-editable[title="padding-right"]{
> +  position: relative; 
> +  left: 4px;
> +}

There is a lot of repeated CSS properties in the last 8 CSS rules.
This needs a bit of cleanup.
Why not move the position:relative property to the .boxmodel-editable { ... } rule, which is further up in the file. This way, no need to repeat every time here.
Also the background-color, padding and border-radius is repeated 4 times, for each of the border element. You could move this to a new rule like this:
.boxmodel-editable[data-box="border"] {
  background-color: #737373;
  border-radius: 3px;
  padding: 0 2px;
}
This way, these 8 last CSS rules could become much simpler because they would only define the top/left relative offsets.

::: devtools/client/themes/tooltips.css
@@ +66,4 @@
>  .devtools-tooltip-css-variable {
>    color: var(--theme-body-color);
>    padding: 2px;
> +  display: flex;

I don't understand why this change is part of this patch. It seems unrelated to the box-model.
Should be removed I guess.

Comment 4

[Lawson C]

Attachment: More finalized patch

More finalized version of this patch

Comment 5

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

Comment on attachment 8950756 [details] [diff] [review]
More finalized patch

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

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +462,4 @@
>          onMouseOver: this.onHighlightMouseOver,
>          onMouseOut: this.props.onHideBoxModelHighlighter,
>        },
> +      displayPosition ? dom.span(

This should be 

displayPosition ?
  dom.span(
    {
       ..
    }
  )
  :
  null,

::: devtools/client/themes/boxmodel.css
@@ +96,3 @@
>  }
>  
> +

Remove this new line.

@@ +288,4 @@
>  }
>  
>  .boxmodel-legend[data-box="margin"] {
> +  margin-left: 9px; 

Trailing whitespace.

@@ +320,5 @@
>  
>  /* Editable fields */
>  
>  .boxmodel-editable {
> +  position: relative; 

Trailing whitespace

@@ +331,5 @@
> +  border-radius: 3px;
> +  padding: 0 2px;
> +}
> +
> +.boxmodel-editable[title="border-top-width"]{

You need a space before the "{" for all of these.

@@ +332,5 @@
> +  padding: 0 2px;
> +}
> +
> +.boxmodel-editable[title="border-top-width"]{
> +  top: -3px;

All of these boxmodel-editable rules seems wrong. You can see that we set these top/left values in rules such as .boxmodel-margin.boxmodel-top and that's what you should be adjusting.

::: devtools/client/themes/variables.css
@@ +285,4 @@
>    /* The photon animation curve */
>    --animation-curve: cubic-bezier(.07,.95,0,1);
>  
> +  /* Box Model Colors */ 

Trailing whitespaces that needs to be trimmed

@@ +285,5 @@
>    /* The photon animation curve */
>    --animation-curve: cubic-bezier(.07,.95,0,1);
>  
> +  /* Box Model Colors */ 
> +  --margins-color: #fdffdf;

I don't think these variables should be here. They should be defined in the :root of boxmodel.css

@@ +289,5 @@
> +  --margins-color: #fdffdf;
> +  --border-color: var(--grey-50);
> +  --paddings-color: #e3dcff;
> +  --contents-color: #cff0fb;
> +  

Trailing whitespaces that need to be removed. I don't think we need this new line

Comment 6

[Lawson C]

Attachment: cssbox

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8951458 [details] [diff] [review]
cssbox

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

Thank you Lawson. This looks pretty good.
I would just like to take one final look after you address the comments I listed below!
Thank you for your patience.

::: devtools/client/themes/boxmodel.css
@@ +6,4 @@
>   * This is the stylesheet of the Box Model view implemented in the layout panel.
>   */
>  
> +:root {

I know Gabriel suggested using the :root selector here, but I'm worried about one thing: all of the panels of the inspector live inside just one document (no nested iframes), so :root{} is actually common to all these panels. However, here, we're defining variables that are only useful for the box-model component. So I would prefer using a more specialized selector to avoir declaring these variables for everything else in the inspector, and risking some overlap.

Can you please change to .boxmodel-container instead?
(by the way, there is already a rule defined with this selector below, so you can just move all the variables inside this rule).

@@ +9,5 @@
> +:root {
> +  --margins-color: #fdffdf;
> +  --border-color: var(--grey-50);
> +  --paddings-color: #e3dcff;
> +  --contents-color: #cff0fb;

Maybe some name changes needed here: margins is plural, while border is singular. Contents is plural but there's only ever one content are.

Here's a proposal:
--marginbox-color
--borderbox-color
--paddingbox-color
--contentbox-color

@@ +12,5 @@
> +  --paddings-color: #e3dcff;
> +  --contents-color: #cff0fb;
> +  
> +  --margins-border-color: #d8e60a;
> +  --contents-border-color: #54A9FF;

same here:
--marginbox-border-color
--contentbox-border-color

@@ +61,2 @@
>  .boxmodel-margin,
> +.boxmodel-padding{

nit: please add a space before the { character.

@@ +80,5 @@
> +  border-style: solid;
> +  outline: dashed 1px var(--margins-border-color);
> +}
> +
> +.boxmodel-borders{

nit: please add a space here too.

@@ +304,4 @@
>  }
>  
>  .theme-dark .boxmodel-legend[data-box="margin"] {
> +  color: var(--grey-90);

The same color is already defined in the rule just above (.boxmodel-legend[data-box="margin"]), so you don't need to redefine it here in theory.

@@ +324,3 @@
>  }
>  
>  .boxmodel-legend[data-box="position"] {

You're defining the position of the legend for each and every boxes here using the margin property.
However, this rule (on line 294):
.boxmodel-legend {
  position: absolute;
  margin: 2px 6px;
  z-index: 1;
}

also defines it, but since you re-define it for every box, the margin in this rule is useless. You should remove it.

@@ +335,5 @@
>    border: 1px dashed transparent;
>    -moz-user-select: none;
>  }
>  
> +.boxmodel-editable[data-box="border"] 

nit: please move the { character to the same line as the character.

Comment 8

[Lawson C]

Attachment: cssbox

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8954601 [details] [diff] [review]
cssbox

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

I think all of my concerns have been addressed. This looks good. Thank you!
Just one final minor clean up to do, but no need to ask for a new review after that. You can do it, attached the new patch, and mark it as R+ again yourself.

::: devtools/client/themes/boxmodel.css
@@ +299,3 @@
>  }
>  
>  .theme-dark .boxmodel-legend[data-box="margin"] {

This rule is now empty. Please remove it.

Comment 10

[Lawson C]

Attachment: cssbox

Comment 11

[Lawson C]

Attachment: cssbox

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

What's the next step here?  Do we need a try run?

Comment 13

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> What's the next step here?  Do we need a try run?

I will push the try run. I needed a rebase patch on Friday because it wasn't applying. I will get to this today.

Comment 14

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

We won't be landing this until after the 60 merge that happens next Monday.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afa73fb650a
Implement the photon redesign of the box model. r=pbro

Comment 16

[Raul Gurzau (:RaulG)]

Backed out changeset 2afa73fb650a (bug 1436549) for devtools failures on /boxmodel/test/browser_boxmodel_guides.js

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c2951dae436864ed6e7115e8805f7d45985630

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2afa73fb650aee5099ca169c5888067a62fc6c2f

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=167860096&repo=mozilla-inbound&lineNumber=2536

[task 2018-03-14T05:19:15.588Z] 05:19:15     INFO - Synthesizing mouseover on the boxmodel-view
[task 2018-03-14T05:19:15.591Z] 05:19:15     INFO - Waiting for the node-highlight event from the toolbox
[task 2018-03-14T05:19:15.592Z] 05:19:15     INFO - Buffered messages finished
[task 2018-03-14T05:19:15.593Z] 05:19:15     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_guides.js | Test timed out - 
[task 2018-03-14T05:19:15.598Z] 05:19:15     INFO - Removing tab.
[task 2018-03-14T05:19:15.599Z] 05:19:15     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-03-14T05:19:15.600Z] 05:19:15     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-03-14T05:19:15.601Z] 05:19:15     INFO - Tab removed and finished closing
[task 2018-03-14T05:19:15.603Z] 05:19:15     INFO - GECKO(1327) | MEMORY STAT | vsize 1019MB | residentFast 311MB | heapAllocated 103MB
[task 2018-03-14T05:19:15.604Z] 05:19:15     INFO - TEST-OK | devtools/client/inspector/boxmodel/test/browser_boxmodel_guides.js | took 45364ms

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0581f0c879
Implement the photon redesign of the box model. r=pbro

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8b0581f0c879