Bug 221972 – Web Inspector: Grid layout labels can be drawn outside the viewport

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

[Patrick Angle]

Steps to reproduce:
1. Navigate to https://labs.jensimmons.com/2017/03-008.html.
2. Open the web inspector and enable the grid overlay for `main`.
3. Disable the authored margin on the `body` element.

Expected Results:
Labels would reposition to not be drawn outside the page.

Actual Results:
Labels are drawn outside the bounds of the page.

[Patrick Angle]

Created attachment 420491 [details]
Screenshot of Issue

[Radar WebKit Bug Importer]

<rdar://problem/74648296>

[Patrick Angle]

Created attachment 424086 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 424086 [details]
Patch v1.0

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

> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:340
> +    CGPath* labelPath = labelWebCorePath.ensurePlatformPath();

I missed a `#if USE(CG)` here... And these two lines should move down to where they are used below the `textPosition` calculations.

[Patrick Angle]

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

[Patrick Angle]

Created attachment 424146 [details]
Patch v1.1 - Added `USE(CG)` check

[Blaze Burg]

Comment on attachment 424146 [details]
Patch v1.1 - Added `USE(CG)` check

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

r=me, great stuff!

> Source/WebKit/ChangeLog:8
> +        Add support the new `WebCore::InspectorOverlay::LabelArrowEdgePosition` property to grid overlays on iOS.

Nit: add support *for*

> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:411
> +    auto labelWebCorePath = WebCore::InspectorOverlay::backgroundPathForLayoutLabel(textWidth + (padding * 2), textHeight + (padding * 2), arrowDirection, arrowEdgePosition, arrowSize);

I would name this labelPath, and the existing labelPath as platformLabelPath

[Patrick Angle]

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

[Patrick Angle]

Created attachment 424376 [details]
Patch v1.3 - Fix typo

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Patrick Angle]

Created attachment 424436 [details]
Patch v1.4 - Added reviewer to changelogs

[EWS]

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

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

[Michael Catanzaro]

This introduced an unused variable warning:

In file included from WebCore/DerivedSources/unified-sources/UnifiedSource-84c9f43f-3.cpp:4:
../../Source/WebCore/inspector/InspectorOverlay.cpp: In member function ‘WTF::Optional<WebCore::InspectorOverlay::Highlight::GridHighlightOverlay> WebCore::InspectorOverlay::buildGridOverlay(const WebCore::InspectorOverlay::Grid&, bool)’:
../../Source/WebCore/inspector/InspectorOverlay.cpp:1768:23: warning: variable ‘lineBetweenColumnBottoms’ set but not used [-Wunused-but-set-variable]
 1768 |             FloatLine lineBetweenColumnBottoms = { columnStartLine.end(), previousColumnEndLine.end() };
      |

It looks like a copy/paste error:

            FloatLine lineBetweenColumnTops = { columnStartLine.start(), previousColumnEndLine.start() };
            FloatLine lineBetweenColumnBottoms = { columnStartLine.end(), previousColumnEndLine.end() };
            gapLabelLine = { lineBetweenColumnTops.pointAtRelativeDistance(0.5), lineBetweenColumnTops.pointAtRelativeDistance(0.5) };

Patrick, can you look at it please?

[Michael Catanzaro]

There's also a new -Wreturn-type warning:

../../Source/WebCore/inspector/InspectorOverlay.cpp: In function ‘WebCore::FloatSize WebCore::expectedSizeForLayoutLabel(WTF::String, WebCore::InspectorOverlay::LabelArrowDirection, float)’:
../../Source/WebCore/inspector/InspectorOverlay.cpp:1349:54: warning: control reaches end of non-void function [-Wreturn-type]
 1349 |     auto font = InspectorOverlay::fontForLayoutLabel();
      |                                                      ^

This is very common since clang doesn't warn when all enum values are covered by the switch, but GCC does. I'll add a RELEASE_ASSERT_NOT_REACHED().

[Patrick Angle]

> Patrick, can you look at it please?

Yep, I’ll take a look and get these fixed today; thank you for bringing these to my attention!

[Michael Catanzaro]

I handled the -Wreturn-type in bug #224756, so that just leaves the copy/paste issue for you to handle.