Bug 222565 – Web Inspector: Grid overlay does not adjust for element transforms (rotation, scale, etc.)

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

[Patrick Angle]

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.

[Radar WebKit Bug Importer]

<rdar://problem/74879289>

[Patrick Angle]

Created attachment 422153 [details]
Patch v1.0

[Patrick Angle]

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

[Patrick Angle]

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

[Patrick Angle]

Created attachment 422322 [details]
Patch v1.1 - Cleanup

[Patrick Angle]

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

[Patrick Angle]

Punting on perspective transforms for the moment, and adding logic to return early if such a transform is applied. A majority of this logic should hold true for perspective transformed grids, as it uses relative lengths for constructing lines between sides of the grid, which may not form a parallelogram when a perspective transform is present.

[Patrick Angle]

Created attachment 422342 [details]
Patch v2.0 - Use renderGrid to transform points

[Patrick Angle]

Created attachment 422344 [details]
Screenshot of Patch v2.0 (Scale, Translate, & Rotate)

[Patrick Angle]

Created attachment 422345 [details]
Screenshot of Patch v2.0 (Perspective)

[Patrick Angle]

Created attachment 422346 [details]
Screenshot of Patch v2.0 (Perspective /w Area Names)

[Blaze Burg]

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!

[Devin Rousso]

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?

[Patrick Angle]

Created attachment 422584 [details]
Patch v2.1 - Review notes, make FloatLine standalone class

[EWS]

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

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