Options that need to be implemented include: - showLineNumbers - showAreaNames - showExtendedGridlines - showTrackSizes - showLineNames
Created attachment 419791 [details] WIP v0.1 - Scrolling, Hatching, & Initial Labels
Created attachment 419792 [details] Screenshot of WIP v0.1
Created attachment 419879 [details] WIP v0.2 - Horiz. Hatch Scrolling, Label Directions, Grid Bounds Issue Fixed
Created attachment 419880 [details] Real-world Screenshot of WIP v0.2
Created attachment 419881 [details] Demo Screenshot of WIP v0.2
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
<rdar://73504774>
Created attachment 420012 [details] Patch v1.0
Created attachment 420013 [details] Screenshot of Patch v1.0
This patch implements Line Numbers and Extended Gridlines. Area names, track sizes, and line names will come in a later patch.
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.
Created attachment 420357 [details] Patch v1.1 - Review notes, text alignment fix
Created attachment 420360 [details] Screenshot of Patch v1.1
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?
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?
Created attachment 420497 [details] Patch v1.2 - Review notes
Committed r272918: <https://commits.webkit.org/r272918> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420497 [details].