Bug 222565

Summary: Web Inspector: Grid overlay does not adjust for element transforms (rotation, scale, etc.)
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, pangle, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot of Issue
none
Patch v1.0
none
Screenshot of Patch v1.0
none
Screenshot of Patch v1.0
none
Patch v1.1 - Cleanup
none
Screenshot of Patch v1.1
none
Patch v2.0 - Use renderGrid to transform points
none
Screenshot of Patch v2.0 (Scale, Translate, & Rotate)
none
Screenshot of Patch v2.0 (Perspective)
none
Screenshot of Patch v2.0 (Perspective /w Area Names)
none
Patch v2.1 - Review notes, make FloatLine standalone class none

Description Patrick Angle 2021-03-01 11:05:12 PST
Created attachment 421846 [details]
Screenshot of Issue

Description:
For elements with a transform (or whose parent is transformed), the overlay to not appropriately transformed.

Steps to Reproduce:
1. Go to labs.jensimmons.com
2. Inspect the page and turn on the grid overlay for `div.header-wrapper`
3. Observe the grid being drawn in the wrong place.

Expected Result:
Grid would be drawn where layout actually occurred, including rotation, scale, etc.
Comment 1 Radar WebKit Bug Importer 2021-03-01 11:05:33 PST
<rdar://problem/74879289>
Comment 2 Patrick Angle 2021-03-03 15:04:32 PST
Created attachment 422153 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-03-03 15:05:00 PST
Created attachment 422154 [details]
Screenshot of Patch v1.0
Comment 4 Patrick Angle 2021-03-03 15:12:51 PST
Created attachment 422155 [details]
Screenshot of Patch v1.0
Comment 5 Patrick Angle 2021-03-04 19:19:54 PST
Created attachment 422322 [details]
Patch v1.1 - Cleanup
Comment 6 Patrick Angle 2021-03-04 19:20:19 PST
Created attachment 422323 [details]
Screenshot of Patch v1.1
Comment 7 Patrick Angle 2021-03-04 19:23:04 PST Comment hidden (obsolete)
Comment 8 Patrick Angle 2021-03-05 00:58:03 PST
Created attachment 422342 [details]
Patch v2.0 - Use renderGrid to transform points
Comment 9 Patrick Angle 2021-03-05 00:58:59 PST
Created attachment 422344 [details]
Screenshot of Patch v2.0 (Scale, Translate, & Rotate)
Comment 10 Patrick Angle 2021-03-05 00:59:15 PST
Created attachment 422345 [details]
Screenshot of Patch v2.0 (Perspective)
Comment 11 Patrick Angle 2021-03-05 00:59:38 PST
Created attachment 422346 [details]
Screenshot of Patch v2.0 (Perspective /w Area Names)
Comment 12 BJ Burg 2021-03-05 10:46:34 PST
Comment on attachment 422342 [details]
Patch v2.0 - Use renderGrid to transform points

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

r=me

Please test this with a grid inside an iframe. File a followup or new patch revision if localToContainer is not enough-it may require localToRoot.

> Source/WebCore/inspector/InspectorOverlay.cpp:1183
> +        for (float y = -topSide.length(); y < leftSide.length(); y += hatchSpacing) {

This is way easier to grok than the scrollOffset version.

> Source/WebCore/inspector/InspectorOverlay.cpp:1456
> +            auto extendedLine = columnStartLine.extendedToBounds(viewportBounds);

Very nice.

> Source/WebCore/inspector/InspectorOverlay.cpp:1614
> +            // If any two lines are coincident with each other, they will not have an intersection, which can occur with extreme `transform: perspective(...)` values.

Wow!
Comment 13 Devin Rousso 2021-03-05 15:34:17 PST
Comment on attachment 422342 [details]
Patch v2.0 - Use renderGrid to transform points

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

r=me as well, nice work!

> Source/WebCore/inspector/InspectorOverlay.cpp:1415
> +    auto* renderGrid = downcast<RenderGrid>(renderer);

NIT: I'd `*downcast<RenderGrid>(renderer)` so you don't have to keep dereferencing

> Source/WebCore/inspector/InspectorOverlay.cpp:1547
> +        if (i < rowHeights.size() && i + 1 != rowPositions.size()) {

NIT: I usually prefer using `i < rowPositions.size()` just in case `i` somehow gets beyond `rowPositions.size()`

> Source/WebCore/inspector/InspectorOverlay.h:170
> +    class FloatLine {

IMO maybe this is worth moving out of inspector code?  I feel like it could be useful elsewhere :)

> Source/WebCore/inspector/InspectorOverlay.h:173
> +        FloatLine(FloatPoint start, FloatPoint end)

`const FloatPoint&`

> Source/WebCore/inspector/InspectorOverlay.h:181
> +        const FloatPoint start() const { return m_start; }
> +        const FloatPoint end() const { return m_end; }

`const FloatPoint&`

> Source/WebCore/inspector/InspectorOverlay.h:201
> +        const FloatLine extendedToBounds(const FloatRect bounds) const

`const FloatRect&`

> Source/WebCore/inspector/InspectorOverlay.h:227
> +            return makeOptional(FloatPoint(

NIT: Would `{{ ... }}` instead of `makeOptional(FloatPoint(...))` here?
Comment 14 Patrick Angle 2021-03-08 10:50:54 PST
Created attachment 422584 [details]
Patch v2.1 - Review notes, make FloatLine standalone class
Comment 15 EWS 2021-03-08 12:55:50 PST
Committed r274096: <https://commits.webkit.org/r274096>

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