Bug 221972 - Web Inspector: Grid layout labels can be drawn outside the viewport
Summary: Web Inspector: Grid layout labels can be drawn outside the viewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks: 224777
  Show dependency treegraph
 
Reported: 2021-02-16 10:19 PST by Patrick Angle
Modified: 2021-04-19 12:09 PDT (History)
7 users (show)

See Also:


Attachments
Screenshot of Issue (3.30 MB, image/png)
2021-02-16 10:19 PST, Patrick Angle
no flags Details
Patch v1.0 (40.74 KB, patch)
2021-03-23 18:54 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.0 (704.79 KB, image/png)
2021-03-23 18:56 PDT, Patrick Angle
no flags Details
Patch v1.1 - Added `USE(CG)` check (41.00 KB, patch)
2021-03-24 09:54 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 - Review notes (41.01 KB, patch)
2021-03-26 11:01 PDT, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1.3 - Fix typo (41.00 KB, patch)
2021-03-26 11:06 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.4 - Added reviewer to changelogs (40.99 KB, patch)
2021-03-26 19:56 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-02-16 10:19:04 PST
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.
Comment 1 Patrick Angle 2021-02-16 10:19:15 PST
Created attachment 420491 [details]
Screenshot of Issue
Comment 2 Radar WebKit Bug Importer 2021-02-23 10:20:15 PST
<rdar://problem/74648296>
Comment 3 Patrick Angle 2021-03-23 18:54:25 PDT
Created attachment 424086 [details]
Patch v1.0
Comment 4 Patrick Angle 2021-03-23 18:56:11 PDT
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.
Comment 5 Patrick Angle 2021-03-23 18:56:42 PDT
Created attachment 424088 [details]
Screenshot of Patch v1.0
Comment 6 Patrick Angle 2021-03-24 09:54:48 PDT
Created attachment 424146 [details]
Patch v1.1 - Added `USE(CG)` check
Comment 7 BJ Burg 2021-03-24 11:43:28 PDT
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
Comment 8 Patrick Angle 2021-03-26 11:01:47 PDT
Created attachment 424374 [details]
Patch v1.2 - Review notes
Comment 9 Patrick Angle 2021-03-26 11:06:11 PDT
Created attachment 424376 [details]
Patch v1.3 - Fix typo
Comment 10 EWS 2021-03-26 15:06:52 PDT
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 11 Patrick Angle 2021-03-26 19:56:51 PDT
Created attachment 424436 [details]
Patch v1.4 - Added reviewer to changelogs
Comment 12 EWS 2021-03-26 20:27:32 PDT
Committed r275128: <https://commits.webkit.org/r275128>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424436 [details].
Comment 13 Michael Catanzaro 2021-04-19 06:43:04 PDT
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?
Comment 14 Michael Catanzaro 2021-04-19 06:46:00 PDT
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().
Comment 15 Patrick Angle 2021-04-19 07:30:23 PDT
> 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!
Comment 16 Michael Catanzaro 2021-04-19 09:19:24 PDT
I handled the -Wreturn-type in bug #224756, so that just leaves the copy/paste issue for you to handle.