1449568 - CSS Grid Inspector: Line-markers should not be cut off

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: Grid Inspector Cut Off Markers.pdf

Line markers in the CSS grid inspector are cut off currently when close to the edge of the viewport. This makes them unusable in some situations.

Instead, the markers should always be visible. When they are close to the viewport edge, they should move from outside the grid to the inside. If required they should alter their shape in tricky corner cases.

Please find examples of the current situation (top row) and mockups of the proposed change (bottom row) in the attached PDF document.

Comment 1

[Patrick Brosset <:pbro>]

Looking at this now. I have a first solution that seems to work ok for now. But I need to test this with more use cases.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1449568 - Grow the arrow box if it's partly hidden, and move the text to be visible;

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

Comment 3

[Patrick Brosset <:pbro>]

I will need people to manually test this and tell me what they think about this.
I believe the proposed fix is an improvement over the current state. We can of course always make this better, but let's see if this is at least making some of the common use cases better.
Martin, here's a try push URL which you should be able to get builds from. If you could test that'd be great.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b2aff4eb7230606920716e7d99303f3a44a855e

Comment 4

[Martin Balfanz [:mbalfanz]]

Attachment: Screen Shot 2018-05-23 at 19.09.18.png

https://lynnandtonic.com/ has 2 arrows at the bottom left corner at around 895px width

Comment 5

[Martin Balfanz [:mbalfanz]]

Attachment: Screen Shot 2018-05-23 at 19.11.32.png

2 arrows stacked at the bottom left corner at around 1415px width

Comment 6

[Patrick Brosset <:pbro>]

Thanks for the feedback. Looking at this now, I can see that there's a 12th implicit row line. I would expect there to be only 11 row lines, all of the explicit. I wonder why there's an extra implicit line at the end.
I'll need more time to investigate.

Comment 7

[Patrick Brosset <:pbro>]

Looks like there's an ::after pseudo-element as the last grid item, and since it doesn't have anywhere to sit in the explicit grid, it creates a new implicit line. However what's weird is that we get 12 lines, but only 10 tracks.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #7)
> Looks like there's an ::after pseudo-element as the last grid item, and
> since it doesn't have anywhere to sit in the explicit grid, it creates a new
> implicit line. However what's weird is that we get 12 lines, but only 10
> tracks.
I was wrong, there is indeed an implicit row track created at the bottom of the grid, but we never get into a situation where getGridFragments returns 12 lines and 10 tracks. I must have imagined it, because I can't reproduce it anymore.

So, attachment 8980005 [details] is expected, because the implicit track exists, but has a breadth of 0px, so the 2 line number markers are stacked.

However attachment 8980004 [details] is incorrect. The 2 markers are expected, but they should appear stacked like in the other attachment.
I know the reason for this: because the marker that is shown as stacked is offset slightly to the left, it ends up being too close to the edge, and so we move it to be inside the grid. But in case of stacked markers, we probably shouldn't.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8979983 [details]
Bug 1449568 - Grow the arrow box if it's partly hidden, and move the text to be visible;

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

Comment 10

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

Comment on attachment 8979983 [details]
Bug 1449568 - Grow the arrow box if it's partly hidden, and move the text to be visible;

https://reviewboard.mozilla.org/r/246168/#review254586

::: devtools/server/actors/highlighters/css-grid.js:78
(Diff revision 2)
>  
> -// 25 is a good margin distance between the document grid container edge without cutting
> -// off parts of the arrow box container.
> -const OFFSET_FROM_EDGE = 25;
> +// This is the minimum distance a line can be to the edge of the document under which we
> +// push the line number arrow to be inside the grid. This offset is enough to fit the
> +// entire arrow + a stacked arrow behind it.
> +const OFFSET_FROM_EDGE = 32;
> +// And this is how much inside the grid we push the arrow. This a factor of the arrow

s/And this/This

::: devtools/server/actors/highlighters/css-grid.js:1231
(Diff revision 2)
>  
>      let minBoxSize = arrowSize * 2 + padding;
>      boxWidth = Math.max(boxWidth, minBoxSize);
>      boxHeight = Math.max(boxHeight, minBoxSize);
>  
> -    // Determine default box edge to aim the line number arrow at.
> +    // Determine which side is the edge of the box to aim the line number arrow at.

I think we can reword this to be:

Determine which edge of the box to aim the line number arrow at.

Since edge refers to a side.

::: devtools/server/actors/highlighters/css-grid.js:1291
(Diff revision 2)
> +      this.ctx.restore();
> +      return;
> +    }
> +
> +    // If the arrow's edge (the one perpendicular to the line direction) is too close to
> +    // the edge of the window. Push the arrow inside the grid.

s/window/viewport to be consistent with our wording above.

::: devtools/server/actors/highlighters/css-grid.js:1358
(Diff revision 2)
> +
> +    // Draw the arrow box itself
>      drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, arrowSize,
>                     boxEdge);
>  
> -    // Adjust position based on the edge.
> +    // Now define the text position for it to be centered nicely inside the arrow box.

s/Now define/Determine

::: devtools/server/actors/highlighters/css-grid.js:1374
(Diff revision 2)
>        case "bottom":
>          y += (boxHeight + arrowSize + radius) - boxHeight / 2;
>          break;
>      }
>  
> +    // And do a second pass to adjust the position, along the other axis, if the box grew

s/And do/Do

::: devtools/server/actors/highlighters/utils/canvas.js:127
(Diff revision 2)
>    ctx.lineTo(x + width, y + radius);
> +  // Draw the top/right rounded corner.
>    ctx.arcTo(x + width, y, x + width - radius, y, radius);
> +  // Go to the left.
>    ctx.lineTo(x + radius, y);
> +  // Draw the top/left rounded corner and we're done.

s/ and we're done/.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8979983 [details]
Bug 1449568 - Grow the arrow box if it's partly hidden, and move the text to be visible;

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c78b7574bc1
Grow the arrow box if it's partly hidden, and move the text to be visible; r=gl

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2c78b7574bc1