Bug 221644 – Web Inspector: CSS Grid - implement grid overlay options

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

Options that need to be implemented include:
 - showLineNumbers
 - showAreaNames
 - showExtendedGridlines
 - showTrackSizes
 - showLineNames

[Patrick Angle]

Created attachment 419791 [details]
WIP v0.1 - Scrolling, Hatching, & Initial Labels

[Patrick Angle]

Created attachment 419792 [details]
Screenshot of WIP v0.1

[Patrick Angle]

Created attachment 419879 [details]
WIP v0.2 - Horiz. Hatch Scrolling, Label Directions, Grid Bounds Issue Fixed

[Patrick Angle]

Created attachment 419880 [details]
Real-world Screenshot of WIP v0.2

[Patrick Angle]

Created attachment 419881 [details]
Demo Screenshot of WIP v0.2

[Blaze Burg]

Comment on attachment 419879 [details]
WIP v0.2 - Horiz. Hatch Scrolling, Label Directions, Grid Bounds Issue Fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=419879&action=review

Great work!

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Nit: remove

> Source/WebCore/inspector/InspectorOverlay.cpp:1159
> +    // Move across the top of the rect

Nit: comments end with '.'

The hatching looks kinda chunky to me, especially on narrow track gaps where the hatching is not very visible. Maybe use a thinner stroke / less spacing?

> Source/WebCore/inspector/InspectorOverlay.cpp:1176
> +    // FIXME: Layout labels can be drawn outside the viewport, and a best effort should be made to keep them in the viewport while the grid is in the viewport.

I think the caller should make this determination.

> Source/WebCore/inspector/InspectorOverlay.cpp:1182
> +    fontDescription.setFamilies({ "Menlo", m_page.settings().fixedFontFamily() });

I don't have strong opinions about the font used, but Menlo is probably less favored than modern fonts like SF which can be fine-tuned.

> Source/WebCore/inspector/InspectorOverlay.cpp:1252
> +    context.drawText(font, TextRun(label), textPosition);

We may want to play around with these color values. Typically we don't use solid white and black in macOS design language.

> Source/WebCore/inspector/InspectorOverlay.cpp:1281
> +    FloatRect gridBoundingBox = FloatRect(absContentQuad.boundingBox().x(), absContentQuad.boundingBox().y(), renderGrid->columnPositions()[renderGrid->columnPositions().size() - 1], renderGrid->rowPositions()[renderGrid->rowPositions().size() - 1]);

If possible, it would be a lot more readable to use an initializer list:

FloatRect gridBoundingBox = {
    absContentQuad.boundingBox().x(),
    absContentQuad.boundingBox().y(),
    renderGrid->columnPositions()[renderGrid->columnPositions().size() - 1],
    renderGrid->rowPositions()[renderGrid->rowPositions().size() - 1]
};

> Source/WebCore/inspector/InspectorOverlay.cpp:1357
> +        

Nit: extra newlines

[Blaze Burg]

<rdar://73504774>

[Patrick Angle]

Created attachment 420012 [details]
Patch v1.0

[Patrick Angle]

Created attachment 420013 [details]
Screenshot of Patch v1.0

[Patrick Angle]

This patch implements Line Numbers and Extended Gridlines. Area names, track sizes, and line names will come in a later patch.

[Devin Rousso]

Comment on attachment 420012 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=420012&action=review

Didn't look too closely at the math, but it seems good (also based on the screenshot).

> Source/WebCore/inspector/InspectorOverlay.cpp:1143
> +void InspectorOverlay::drawHatch(GraphicsContext& context, FloatRect rect, IntPoint scrollOffset)

Do you expect this code to be general-purpose or is it intended to only be used for grid?  If the latter, please add `Grid` or `Layout` somewhere in the name (e.g. `drawGridHatch`).

> Source/WebCore/inspector/InspectorOverlay.cpp:1155
> +    DashArray lineDash;
> +    lineDash.append(2);
> +    lineDash.append(2);
> +    context.setLineDash(lineDash, 2);

I totally forgot about `DashArray`!  I thought you were gonna have to do this logic manually 🤣

> Source/WebCore/inspector/InspectorOverlay.cpp:1157
> +    int hatchSpacing = 12;

`constexpr auto hatchSpacing = 12;`

> Source/WebCore/inspector/InspectorOverlay.cpp:1160
> +    // FIXME: The these two loops interfere with each other at the top left corner of the rect, causing a drawing artifact.

Yeah I had a similar issue when I added the code to draw rulers.  Look for `cornerX`/`cornerY` in `InspectorOverlay::drawRulers` to see how I did it if you'd like some prior-art/inspiration :)

> Source/WebCore/inspector/InspectorOverlay.cpp:1162
> +    for (float x = (-scrollOffset.x() % hatchSpacing) - rect.height(); x < rect.width(); x += hatchSpacing) {

Subtracting `rect.height()` from `x` seems wrong?  If this is intended, I think this is confusing enough to warrant a comment as to why a Y-axis value is used while moving across the X-axis.

> Source/WebCore/inspector/InspectorOverlay.cpp:1164
> +        hatchPath.addLineTo({ x + rect.width(), rect.width()});

ditto (:+1162)

> Source/WebCore/inspector/InspectorOverlay.cpp:1168
> +    for (float y = (-scrollOffset.y() % hatchSpacing) - rect.width(); y < rect.height(); y += hatchSpacing) {

ditto (:+1162)

> Source/WebCore/inspector/InspectorOverlay.cpp:1170
> +        hatchPath.addLineTo({ rect.height(), y + rect.height()});

ditto (:+1162)

> Source/WebCore/inspector/InspectorOverlay.cpp:1192
> +    float padding = 4;
> +    float arrowSize = 4;

ditto (:+1157)

> Source/WebCore/inspector/InspectorOverlay.cpp:1206
> +        labelPath.closeSubpath();

NIT: you could move this outside the `switch` since it's repeated in all of them

> Source/WebCore/inspector/InspectorOverlay.cpp:1244
> +    // FIXME: Reverse black/white for dark mode.

BTW we actually don't have any special styles for dark mode in the page overlay, so I dunno if we want/need this.  We usually try to pick colors that are readable no matter what.

> Source/WebCore/inspector/InspectorOverlay.cpp:1245
> +    // FIXME: Don't use solid black/white.

Why not?

> Source/WebCore/inspector/InspectorOverlay.cpp:1250
> +    // FIXME: Text is not visually vertically centered inside labels.

Yeah I was gonna comment on this.  This definitely should be fixed before this lands as it _really_ stands out :(

If it helps, I'd take a look at `InspectorOverlay::drawElementTitle` as it also has to draw text inside a box :P

> Source/WebCore/inspector/InspectorOverlay.cpp:1278
>      FloatQuad absContentQuad = renderer->localToAbsoluteQuad(FloatRect(contentBox));

Style: This should probably be renamed to `absoluteContentQuad`.

> Source/WebCore/inspector/InspectorOverlay.cpp:1286
> +        renderGrid->rowPositions()[renderGrid->rowPositions().size() - 1]

Style: I'm pretty sure you can have trailing commas in initializer lists.  If so, please add one.

[Patrick Angle]

Created attachment 420357 [details]
Patch v1.1 - Review notes, text alignment fix

[Patrick Angle]

Created attachment 420360 [details]
Screenshot of Patch v1.1

[Devin Rousso]

Comment on attachment 420357 [details]
Patch v1.1 - Review notes, text alignment fix

View in context: https://bugs.webkit.org/attachment.cgi?id=420357&action=review

r=me

> Source/WebCore/inspector/InspectorOverlay.cpp:1155
> +    context.setLineDash(lineDash, 2);

NIT: Could you just inline `{2, 2}` instead of actually creating and manipulating a `DashArray` variable?

> Source/WebCore/inspector/InspectorOverlay.cpp:1160
> +    // The opposite axis' size is used to determine how far to draw a hatch line in both dimensions, which keeps the lines at a 45° angle.

lol i'd just say "deg" instead of using the symbol :P

> Source/WebCore/inspector/InspectorOverlay.cpp:1279
> +    // FIXME: The position of the grid's bounding box can end up in the wrong place in some layout contexts (e.g. inside another grid)

please create a bug and include its URL for this so it's not forgotten :)

> Source/WebCore/inspector/InspectorOverlay.cpp:1308
> +    // FIXME: This clipping clips labels as well.

ditto (:+1279)

> Source/WebCore/inspector/InspectorOverlay.cpp:1340
> +            // FIXME: Layout labels can be drawn outside the viewport, and a best effort should be made to keep them in the viewport while the grid is in the viewport.

ditto (:+1279)

btw you could probably fix this one quickly just by using `std::min`/`std::max` with `viewportSize` on the `point` provided to `drawLayoutLabel` (though you might also need to take into account the width/height of the text and padding and whatnot, but it shouldn't be more than some simple math)

> Source/WebCore/inspector/InspectorOverlay.cpp:1372
> +            // FIXME: Layout labels can be drawn outside the viewport, and a best effort should be made to keep them in the viewport while the grid is in the viewport.

ditto (:+1340)

> Source/WebCore/inspector/InspectorOverlay.h:128
> +    enum class LabelArrowDirection {

NIT: Should this be in `private:` since it's only used by private methods/members?

[Blaze Burg]

Comment on attachment 420357 [details]
Patch v1.1 - Review notes, text alignment fix

Great work, r=me as well. I think it the hatching looks a bit spaced out, does it look better or worse with smaller line spacing?

[Patrick Angle]

Created attachment 420497 [details]
Patch v1.2 - Review notes

[EWS]

Committed r272918: <https://commits.webkit.org/r272918>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420497 [details].