1432029 - Highlight the justify-content area in the flexbox highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Zhenghong Qian]

Attachment: 1432029.patch

still working on it

Comment 2

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

Comment on attachment 8944289 [details] [diff] [review]
1432029.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +38,4 @@
>  const FLEXBOX_CONTAINER_PATTERN_HEIGHT = 14; // px
>  const FLEXBOX_CONTAINER_PATTERN_LINE_DISH = [5, 3]; // px
>  const BASIS_FILL_COLOR = "rgb(109, 184, 255, 0.4)";
> +const JUSTIFY_CONTENT_COLOR = "rgb(237, 214, 255,0.4)";

NIT: Add a space after the comma between 255,0.4

@@ +329,5 @@
>  
> +  /**
> +   * Renders the justify content for a given flex item (left, top, right, bottom) position.
> +   */
> +  renderJustifyContent(justLeft, justTop, justRight, justBottom) {

Remove the "just" prefix for these bounds and simply call it left, top, right, bottom

@@ +356,4 @@
>      let { bounds } = this.currentQuads.content[0];
>      let flexItems = this.currentNode.children;
>  
> +    // Initialize justify-content area by pixels

s/by pixels/in px

@@ +356,5 @@
>      let { bounds } = this.currentQuads.content[0];
>      let flexItems = this.currentNode.children;
>  
> +    // Initialize justify-content area by pixels
> +    let justLeft = 0;

s/just/justify/g

@@ +358,5 @@
>  
> +    // Initialize justify-content area by pixels
> +    let justLeft = 0;
> +    let justRight = 0;
> +    let justBottom = bounds.bottom - bounds.top;

The top and bottoms justify-content area bounds are bounded by the crossStart (top) and crossStart + crossSize (bottom) of a flex line.

@@ +381,4 @@
>        drawRect(this.ctx, left, top, right, bottom, this.currentMatrix);
>        this.ctx.stroke();
>  
> +      // highlight flex-basis

This comment is probably not needed since we can easily read renderFlexItemBasis and know it is highlighting the flex-basis

@@ +385,3 @@
>        this.renderFlexItemBasis(flexItem, left, top, right, bottom, bounds.width);
> +
> +      // highlight justify-content

Remove this comment - same reason as above.

@@ +392,3 @@
>      }
>  
> +    // highlight the last one of justify-content areas

Reword this to

Render the last justify-content areas.

Comment 3

[Zhenghong Qian]

Attachment: 1432029.patch

Comment 4

[Zhenghong Qian]

Attachment: 1432029.patch

Comment 5

[Zhenghong Qian]

Attachment: 1432029.patch

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8945334 [details] [diff] [review]
1432029.patch

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

Thanks. 
My main concern here is about UI design and code clarity.
I'm fine with landing code with this UI design though, and then we can iterate later, but I'd like the code to be clearer. See below:

The renderFlexItems function contains most of the code that draws the justify-content areas. I would have expected the renderJustifyContent to do that.
In fact the renderJustifyContent function only draws a rectangle, and then later, it partially gets cleared.
It would be much easier to follow if that whole logic was in renderJustifyContent instead, and renderFlexItem would just call it.
Especially, the drawRect and clearRect should be in the same function.
Furthermore, if we do decide to change the design for this later, it's just much easier if the whole code drawing justify-content is in just one function.

::: devtools/server/actors/highlighters/flexbox.js
@@ +331,5 @@
> +   * Renders the justify content for a given flex item (left, top, right, bottom) position.
> +   */
> +  renderJustifyContent(left, top, right, bottom) {
> +    this.ctx.fillStyle = JUSTIFY_CONTENT_COLOR;
> +    drawRect(this.ctx, left, top, right, bottom, this.currentMatrix);

Regarding the design here, I'm not convinced that this works.
It's hard for me to understand, as a user, what this background color means, and it's very similar to the flex-basis background color.
I think we need to draw an arrow instead.
But that can be done later in a separate bug and requires discussion.

@@ +393,5 @@
> +        crossSize = Math.round(crossSize);
> +        crossStart = Math.round(crossStart);
> +
> +        if (computedStyle.getPropertyValue("flex-direction") === "column" ||
> +            computedStyle.getPropertyValue("flex-direction") === "column-reverse") {

This is being accessed 4 times in the same function: computedStyle.getPropertyValue("flex-direction")

So, we should move this to somewhere earlier in the function as a local variable and then just reused.

let computedStyle = getComputedStyle(this.currentNode);
let direction = computedStyle.getPropertyValue("flex-direction");

This makes the code simpler and more performant.

@@ +396,5 @@
> +        if (computedStyle.getPropertyValue("flex-direction") === "column" ||
> +            computedStyle.getPropertyValue("flex-direction") === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let computedStyle2 = getComputedStyle(flexItem);

This should be done right at the start of the for (let flexItem of flexItems) loop, so that you don't do it for every line. And so that you don't also do it in the else part of this if.
It should done just once per item.
Also, please use a more self-explanatory variable name. computedStyle2 and 3 don't carry much meaning. 
Something like flexItemComputedStyle is better.

Comment 7

[Zhenghong Qian]

Attachment: Bug 1432029 - Highlight the justify-content area in the flexbox highlighter.

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8948270 [details]
Bug 1432029 - Highlight the justify-content area in the flexbox highlighter.

https://reviewboard.mozilla.org/r/217768/#review223502


Static analysis found 18 defects in this patch.
 - 18 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/highlighters/flexbox.js:223
(Diff revision 1)
> +
> +    if (gCachedFlexboxPattern.has(JUSTIFY_CONTENT)) {
> +      return gCachedFlexboxPattern.get(JUSTIFY_CONTENT);
> +    }
> +
> +    // Create the inversed diagonal lines pattern for the rendering the justify content gaps.

Error: Line 223 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:524
(Diff revision 1)
> +        let { crossStart, crossSize } = flexLine;
> +        crossSize = Math.round(crossSize);
> +        crossStart = Math.round(crossStart);
> +
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {

Error: Line 524 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:524
(Diff revision 1)
> +        let { crossStart, crossSize } = flexLine;
> +        crossSize = Math.round(crossSize);
> +        crossStart = Math.round(crossStart);
> +
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:526
(Diff revision 1)
> +        crossStart = Math.round(crossStart);
> +
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));

Error: Line 526 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:527
(Diff revision 1)
> +
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));

Error: Line 527 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:528
(Diff revision 1)
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);

Error: Line 528 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:528
(Diff revision 1)
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:528
(Diff revision 1)
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:528
(Diff revision 1)
> +        if (direction === "column" || direction === "column-reverse") {
> +          if ((crossStart <= left) && (left <= right) && (right <= crossSize+crossStart)) {
> +            // Remove the margin area for justify-content
> +            let marginTop = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-top")));
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:532
(Diff revision 1)
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);
> +            break;
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {

Error: Line 532 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:532
(Diff revision 1)
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);
> +            break;
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {

Error: Unexpected if as the only statement in an else block. [eslint: no-lonely-if]

::: devtools/server/actors/highlighters/flexbox.js:532
(Diff revision 1)
> +            let marginBottom = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-bottom")));
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);
> +            break;
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:533
(Diff revision 1)
> +            clearRect(this.ctx, crossStart, top-marginTop, crossSize+crossStart, bottom+marginBottom, this.currentMatrix);
> +            break;
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));

Error: Line 533 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:534
(Diff revision 1)
> +            break;
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));
> +            let marginRight = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-right")));

Error: Line 534 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:535
(Diff revision 1)
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));
> +            let marginRight = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-right")));
> +            clearRect(this.ctx, left-marginLeft, crossStart, right+marginRight, crossSize+crossStart, this.currentMatrix);

Error: Line 535 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/actors/highlighters/flexbox.js:535
(Diff revision 1)
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));
> +            let marginRight = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-right")));
> +            clearRect(this.ctx, left-marginLeft, crossStart, right+marginRight, crossSize+crossStart, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:535
(Diff revision 1)
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));
> +            let marginRight = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-right")));
> +            clearRect(this.ctx, left-marginLeft, crossStart, right+marginRight, crossSize+crossStart, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/server/actors/highlighters/flexbox.js:535
(Diff revision 1)
> +          }
> +        } else {
> +          if ((crossStart <= top) && (top <= bottom) && (bottom <= crossSize+crossStart)) {
> +            let marginLeft = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-left")));
> +            let marginRight = Math.round(parseFloat(flexItemComputedStyle.getPropertyValue("margin-right")));
> +            clearRect(this.ctx, left-marginLeft, crossStart, right+marginRight, crossSize+crossStart, this.currentMatrix);

Error: Infix operators must be spaced. [eslint: space-infix-ops]

Comment 9

[Zhenghong Qian]

Comment on attachment 8948270 [details]
Bug 1432029 - Highlight the justify-content area in the flexbox highlighter.

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

Comment 10

[Zhenghong Qian]

Attachment: 1432029.patch

Comment 11

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

Comment on attachment 8948280 [details] [diff] [review]
1432029.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +475,4 @@
>      this.ctx.restore();
>    }
>  
> +  renderJudtifyContent() {

s/renderJudtifyContent/renderJustifyContent

@@ +491,5 @@
> +    // highlight all the contents first:
> +    for (let flexLine of flexLines) {
> +      let { crossStart, crossSize } = flexLine;
> +
> +      if (direction === "column" || direction === "column-reverse") {

This can be simplified to direction.startsWith("column")

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8948280 [details] [diff] [review]
1432029.patch

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

Looks good.
I'm R+ this with the condition that we fix bug 1435749 in the same nightly cycle.
Thanks for your work. I tested this locally. I like it!

::: devtools/server/actors/highlighters/flexbox.js
@@ +480,5 @@
> +      return;
> +    }
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;

No need to get these items manually, the API you use just below has the items.
Example:
this.currentNode.getAsFlexContainer()[0].getItems() 
would return all items in the first line.
So you can remove this line and get the items later.

@@ +498,5 @@
> +        this.drawJustifyContent(0, crossStart, bounds.width, crossStart + crossSize);
> +      }
> +    }
> +
> +    for (let flexItem of flexItems) {

So, here for instance, you can get the items with line.getItems().
Also, the nice thing is that this API works correctly for every case: even if flex items are pseudo-elements, text nodes, or nested inside display:contents elements.
However, this.currentNode.children does not work correctly in those cases.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1435749 for reference.

I just talked with Gabriel, and agreed that it might be better to just refactor the whole thing in bug 1435749 instead. So I guess I'm good with using .children above, granted we fix it later in that other bug.

@@ +513,5 @@
> +      let bottom = Math.round(flexItemBounds.bottom - bounds.top);
> +      let flexItemComputedStyle = getComputedStyle(flexItem);
> +
> +      // clearing the ocupied and margin areas of flexItem:
> +      for (let flexLine of flexLines) {

The other advantage of using the getItems API mentioned above is that you won't need to iterate first on items and then on lines. You'll do it the other way around. And I believe this will simplify this code a bunch.

Comment 13

[Patrick Brosset <:pbro>]

Ah, I found a bug with the margin approach, but it's not really due to your patch, but rather to bug 1298008:
- go to about:newtab
- select the .outer-wrapper element
- toggle the flexbox highlighter

--> it shows 2 justify-content areas on each side of the flex item which sits in the center.
This is incorrect, because justify-content isn't set to center, it is set to normal (which is start, its default value).
The reason the justify-content areas are drawn on each side is because the item has an "auto" horizontal margin, and because of bug 1298008, auto horizontal margins aren't reported correctly by getBoxQuads.

Comment 14

[Zhenghong Qian]

Attachment: 1432029.patch

changed the typo Judtify to Justify.
use startswith in to check "column" and "column-reverse" flex-direction as Gabriel mentioned.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c374077bcfe
Highlight the justify-content area in the flexbox highlighter. r=gl

Comment 16

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/1c374077bcfe