Closed Bug 1432036 Opened 2 years ago Closed 2 years ago

Remove overlapping flex lines with the flex container in flexbox highlighter

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: gl, Assigned: mashuoyi111)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: gl → mashuoyi111
Attached patch 1432036.patch (obsolete) — Splinter Review
Attachment #8944966 - Flags: review?(gl)
Attachment #8944966 - Flags: review?(gl) → review?(pbrosset)
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.
Attachment #8944966 - Flags: review?(pbrosset) → feedback+
(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.
Attached patch 1432036.patch (obsolete) — Splinter Review
Attachment #8950039 - Flags: review?(gl)
Attachment #8944966 - Attachment is obsolete: true
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.
Attachment #8950039 - Flags: review?(gl)
Attachment #8950039 - Attachment is obsolete: true
Attached patch 1432036.patch (obsolete) — Splinter Review
Attachment #8955374 - Flags: review?(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.
Attachment #8955374 - Flags: review?(gl)
Attached patch 1432036.patchSplinter Review
Attachment #8955374 - Attachment is obsolete: true
Attachment #8956698 - Flags: review?(gl)
Attachment #8956698 - Flags: review?(gl) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7939ec786705
Remove overlapping flex lines with the flex container in flexbox highlighter. r=gl
https://hg.mozilla.org/mozilla-central/rev/7939ec786705
https://hg.mozilla.org/mozilla-central/rev/2c5754dcffa9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1452503
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.