1432035 - Visualize the align-items in the flexbox highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Shuoyi Ma]

Attachment: 1432035.patch

Comment 2

[Shuoyi Ma]

Attachment: 1432035.patch

Comment 3

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

Comment on attachment 8944288 [details] [diff] [review]
1432035.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +258,5 @@
>      if (isTopLevel) {
>        this.hide();
>      }
>    }
> +  renderFlexAlignItems() {

Rename this to renderAlignItemLine

@@ +263,5 @@
> +    if (!this.currentQuads.content || !this.currentQuads.content[0]) {
> +      return;
> +    }
> +
> +    let computedStyle = getComputedStyle(this.currentNode);

Move the declaration of computedStyle and flexLines below the bounds assignment in line 283.

@@ +287,5 @@
> +
> +      switch (computedStyle.getPropertyValue("align-items")) {
> +        case "flex-start":
> +        case "start":
> +          if (computedStyle.getPropertyValue("flex-direction") === "column" ||

Maybe we can simplify the check for "column" and "column-reverse" by using 

computedStyle.getPropertyValue("flex-direction").startsWith("column")

@@ +295,5 @@
> +            this.ctx.stroke();
> +          } else {
> +            drawRect(this.ctx, 0, crossStart, bounds.width, crossStart,
> +              this.currentMatrix);
> +            this.ctx.stroke();

We can simply move this.ctx.stroke() outside the if statement so we can just call it once.

@@ +301,5 @@
> +
> +          break;
> +        case "flex-end":
> +        case "end":
> +          if (computedStyle.getPropertyValue("flex-direction") === "column" ||

computedStyle.getPropertyValue("flex-direction").startsWith("column")

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

Move this.ctx.stroke() out.

@@ +315,5 @@
> +
> +          break;
> +        case "center":
> +        case "normal":
> +          if (computedStyle.getPropertyValue("flex-direction") === "column" ||

computedStyle.getPropertyValue("flex-direction").startsWith("column")

@@ +323,5 @@
> +            this.ctx.stroke();
> +          } else {
> +            drawRect(this.ctx, 0, crossStart + crossSize / 2, bounds.width,
> +            crossStart + crossSize / 2, this.currentMatrix);
> +            this.ctx.stroke();

Move this.ctx.stroke() out.

Comment 4

[Shuoyi Ma]

Attachment: 1432035.patch

I did not apply the if statement changes since the bug 1432036 does not fit the change.

Comment 5

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

Comment on attachment 8946102 [details] [diff] [review]
1432035.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +248,5 @@
>     * the cached gap patterns and avoid using DeadWrapper obejcts as gap patterns the
>     * next time.
>     */
>    onWillNavigate({ isTopLevel }) {
>      this.clearCache();

Don't remove this line

@@ +279,5 @@
> +
> +    for (let flexLine of flexLines) {
> +      let { crossStart, crossSize } = flexLine;
> +
> +      switch (computedStyle.getPropertyValue("align-items")) {

We can save the align-items property value in a variable after computedStyle to avoid getting this multiple times.

@@ +282,5 @@
> +
> +      switch (computedStyle.getPropertyValue("align-items")) {
> +        case "flex-start":
> +        case "start":
> +          if (computedStyle.getPropertyValue("flex-direction").startsWith("column")) {

Same as above we can add a isColumn variable

@@ +330,5 @@
>  
>    renderFlexContainerBorder() {
>      if (!this.currentQuads.content || !this.currentQuads.content[0]) {
>        return;
>      }

Don't remove this line

@@ +512,5 @@
>      // Update the current matrix used in our canvas' rendering
>      let { currentMatrix, hasNodeTransformations } = getCurrentMatrix(this.currentNode,
>        this.win);
>      this.currentMatrix = currentMatrix;
>      this.hasNodeTransformations = hasNodeTransformations;

Don't remove this line

@@ +522,1 @@
>      this._showFlexbox();

Don't remove this line

Comment 6

[Shuoyi Ma]

Attachment: 1432035.patch

Comment 7

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

Comment on attachment 8949231 [details] [diff] [review]
1432035.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +270,5 @@
> +    let canvasY = Math.round(this._canvasPosition.y * devicePixelRatio);
> +
> +    this.ctx.save();
> +    this.ctx.translate(offset - canvasX, offset - canvasY);
> +    this.ctx.setLineDash(FLEXBOX_LINES_PROPERTIES.alignItems.lineDash);

FLEXBOX_LINES_PROPERTIES.alignItems doesn't exist. So, the patch doesn't actually work.

@@ +283,5 @@
> +    for (let flexLine of flexLines) {
> +      let { crossStart, crossSize } = flexLine;
> +
> +      switch (alignItemsType) {
> +        case "flex-start":

We need to handle all the align items type. See https://developer.mozilla.org/en-US/docs/Web/CSS/align-items.

@@ +313,5 @@
> +        case "center":
> +        case "normal":
> +          if (isColumn) {
> +            drawRect(this.ctx, crossStart + crossSize / 2, 0,
> +            crossStart + crossSize / 2, bounds.height, this.currentMatrix);

Indent this

@@ +317,5 @@
> +            crossStart + crossSize / 2, bounds.height, this.currentMatrix);
> +            this.ctx.stroke();
> +          } else {
> +            drawRect(this.ctx, 0, crossStart + crossSize / 2, bounds.width,
> +            crossStart + crossSize / 2, this.currentMatrix);

Indent this

Comment 8

[Shuoyi Ma]

Attachment: 1432035.patch

Comment 9

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

Comment on attachment 8949633 [details] [diff] [review]
1432035.patch

Looks like this patch is for the wrong bug. Clearing the review

Comment 10

[Shuoyi Ma]

Attachment: 1432035.patch

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a2139264a3
Visualize the align-items in the flexbox highlighter. r=gl

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e2a2139264a3