Closed Bug 1449568 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: mbalfanz, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Blocks: dt-grid
Priority: -- → P3
Depends on: 1449528
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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
Flags: needinfo?(mbalfanz)
https://lynnandtonic.com/ has 2 arrows at the bottom left corner at around 895px width
Flags: needinfo?(mbalfanz)
2 arrows stacked at the bottom left corner at around 1415px width
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.
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.
(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 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/.
Attachment #8979983 - Flags: review?(gl) → review+
Pushed by pbrosset@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2c78b7574bc1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.