1432036 - Remove overlapping flex lines with the flex container in flexbox highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

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

When we render the flex lines, we draw the top and bottom of the flex line. This can overlap with the flexbox container border line. This is to remove the overlapping of the flex lines with the flexbox container border line.

Comment 1

[Shuoyi Ma]

Attachment: 1432036.patch

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8944966 [details] [diff] [review]
1432036.patch

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

I like the general approach.
I do have a couple of comments about the code.
And one more general comment too: it's not immediately obvious why we do this. So I think we could use a comment in the code that says something like: "Avoid drawing the start and end lines of flex lines so they don't overlap with the containers".

::: devtools/server/actors/highlighters/flexbox.js
@@ +398,5 @@
>            computedStyle.getPropertyValue("flex-direction") === "column-reverse") {
>          clearRect(this.ctx, crossStart, 0, crossStart + crossSize, bounds.height,
>            this.currentMatrix);
> +        if (crossStart != 0) {
> +          drawRect(this.ctx, crossStart, 0, crossStart, bounds.height,

I was very confused at first as to why we were drawing 2 rectangles, but I understand now that we use the drawRect function to draw lines.
I think we should make this code clearer, by creating a drawLine function in the /devtools/server/actors/highlighters/utils/canvas.js file, so that it's clearer what's happening here.

@@ +402,5 @@
> +          drawRect(this.ctx, crossStart, 0, crossStart, bounds.height,
> +            this.currentMatrix);
> +          this.ctx.stroke();
> +        }
> +        if (bounds.width - crossStart - crossSize >= lineWidth) {

Can you explain what this if statement does?
I don't understand why you need to compare to lineWidth here.
It looks like the way we draw the flex lines right now is a bit wrong, because they overflow the flex container. Which I guess is why you need to do >= lineWidth.
If we fixed the way we drew flex lines in a way that they didn't overflow the containers, then we wouldn't have to do this.

Comment 3

[Shuoyi Ma]

(In reply to Patrick Brosset <:pbro> from comment #2)
> Comment on attachment 8944966 [details] [diff] [review]
> 1432036.patch
> 
> Review of attachment 8944966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the general approach.
> I do have a couple of comments about the code.
> And one more general comment too: it's not immediately obvious why we do
> this. So I think we could use a comment in the code that says something
> like: "Avoid drawing the start and end lines of flex lines so they don't
> overlap with the containers".
> 
> ::: devtools/server/actors/highlighters/flexbox.js
> @@ +398,5 @@
> >            computedStyle.getPropertyValue("flex-direction") === "column-reverse") {
> >          clearRect(this.ctx, crossStart, 0, crossStart + crossSize, bounds.height,
> >            this.currentMatrix);
> > +        if (crossStart != 0) {
> > +          drawRect(this.ctx, crossStart, 0, crossStart, bounds.height,
> 
> I was very confused at first as to why we were drawing 2 rectangles, but I
> understand now that we use the drawRect function to draw lines.
> I think we should make this code clearer, by creating a drawLine function in
> the /devtools/server/actors/highlighters/utils/canvas.js file, so that it's
> clearer what's happening here.
> 
> @@ +402,5 @@
> > +          drawRect(this.ctx, crossStart, 0, crossStart, bounds.height,
> > +            this.currentMatrix);
> > +          this.ctx.stroke();
> > +        }
> > +        if (bounds.width - crossStart - crossSize >= lineWidth) {
> 
> Can you explain what this if statement does?
> I don't understand why you need to compare to lineWidth here.
> It looks like the way we draw the flex lines right now is a bit wrong,
> because they overflow the flex container. Which I guess is why you need to
> do >= lineWidth.
> If we fixed the way we drew flex lines in a way that they didn't overflow
> the containers, then we wouldn't have to do this.

We should have a render drawLine function for canvas.js. Maybe we can create a new task for this since this needs some code refactor? for the If statement, my approach is to avoid drawing the line when distance between the flex box container and the flex line is smaller than the line width. In other words, when two lines so close that about to touch each other, don't drawing the flex line.

Comment 4

[Shuoyi Ma]

Attachment: 1432036.patch

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8950039 [details] [diff] [review]
1432036.patch

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

Stealing this from Gabriel since I did the first review.
This looks a lot easier to read with the drawLine function.
I only have a few remaining comments before giving R+:

- can you add a simple comment in the code explaining why we do this. Something like:
// Avoid drawing the start and end lines of flex lines when they overlap with the containers'

- see my comments in the code below:

::: devtools/server/actors/highlighters/flexbox.js
@@ -386,5 @@
>      this.ctx.save();
>      this.ctx.translate(offset - canvasX, offset - canvasY);
>      this.ctx.lineWidth = lineWidth;
>      this.ctx.strokeStyle = DEFAULT_COLOR;
> -

nit: please do not remove this empty line, it helps with ready the code better.

@@ +391,5 @@
>      let { bounds } = this.currentQuads.content[0];
>      let computedStyle = getComputedStyle(this.currentNode);
>      let flexLines = this.currentNode.getAsFlexContainer().getLines();
> +    let options = new Object();
> +    options.matrix = this.currentMatrix;

No need to use the Object constructor here, you can simply do this:
let options = { matrix: this.currentMatrix };

@@ -415,3 @@
>        }
>      }
> -

nit: please do not remove this empty line, it helps with reading the code better.

@@ -418,3 @@
>      this.ctx.restore();
>    }
> -

Same here, this empty line between the 2 functions is useful.

Comment 6

[Shuoyi Ma]

Attachment: 1432036.patch

Comment 7

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

Comment on attachment 8955374 [details] [diff] [review]
1432036.patch

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

Patch needs to be rebased.

::: devtools/server/actors/highlighters/flexbox.js
@@ +391,5 @@
>  
>      let { bounds } = this.currentQuads.content[0];
>      let computedStyle = getComputedStyle(this.currentNode);
>      let flexLines = this.currentNode.getAsFlexContainer().getLines();
> +    let option = { matrix: this.currentMatrix };

This should be options.

@@ +400,5 @@
>        if (computedStyle.getPropertyValue("flex-direction") === "column" ||
>            computedStyle.getPropertyValue("flex-direction") === "column-reverse") {
>          clearRect(this.ctx, crossStart, 0, crossStart + crossSize, bounds.height,
>            this.currentMatrix);
> +        if (crossStart != 0) {

Add a new line before this

@@ +404,5 @@
> +        if (crossStart != 0) {
> +          drawLine(this.ctx, crossStart, 0, crossStart, bounds.height,
> +            options);
> +          this.ctx.stroke();
> +        }

Add a new line after this

@@ +414,4 @@
>        } else {
>          clearRect(this.ctx, 0, crossStart, bounds.width, crossStart + crossSize,
>            this.currentMatrix);
> +        if (crossStart != 0) {

Add a new line before this.

@@ +416,5 @@
>            this.currentMatrix);
> +        if (crossStart != 0) {
> +          drawLine(this.ctx, 0, crossStart, bounds.width, crossStart, options);
> +          this.ctx.stroke();
> +        }

Add a new line after this.

Comment 8

[Shuoyi Ma]

Attachment: 1432036.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7939ec786705
Remove overlapping flex lines with the flex container in flexbox highlighter. r=gl

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5754dcffa9
Fix eslint error in flexbox.js. r=me

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7939ec786705
https://hg.mozilla.org/mozilla-central/rev/2c5754dcffa9