Bug 236738 – Web Inspector: [Flexbox] Add indicators for layout context in element tooltips

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

[Patrick Angle]

<rdar://87893171>

[Patrick Angle]

Created attachment 452264 [details]
Patch v1.0

[Patrick Angle]

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

[Devin Rousso]

Comment on attachment 452264 [details]
Patch v1.0

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

overall the logic looks good, but I have some design questions/concerns

> Source/WebCore/en.lproj/Localizable.strings:482
> +"Flex Container (Inspector Element Tooltip)" = "Flex Container";

NIT: perhaps "Inspector Element Selection" (since "Start Element Eelection" is the name of the menubar action)

> Source/WebCore/inspector/InspectorOverlay.cpp:1026
> +    if (!is<RenderFlexibleBox>(parentRenderer))

FYI we now have `dynamicDowncast` 🤩
```
if (auto* parentFlexRenderer = dynamicDowncast<RenderFlexibleBox>(renderer.parent()))
    return !parentFlexRenderer->orderIterator().shouldSkipChild(renderer);

return false;
```

> Source/WebCore/inspector/InspectorOverlay.cpp:1089
> +            layoutContextInfo.append(" + "_s);

I kinda feel like we should localize this 🤔

Also, rather than adding a `" + "`, why not have separate badges?  I think that would make it easier to add new things in the future if we wanted (i.e. we probably wouldn't want "Flex Item + Flex Container + Other Thing 1 + Other Thing 2").

> Source/WebCore/inspector/InspectorOverlay.cpp:1094
> +        appendToLayoutContextInfo(WEB_UI_STRING_KEY("Flex Item", "Flex Item (Inspector Element Tooltip)", "Inspector element tooltip text for items inside a Flexbox Container."));

Do we not want a "Grid Item" badge too?

> Source/WebCore/inspector/InspectorOverlay.cpp:1116
> +    Vector<InspectorOverlayLabel::Content> labelContents;

NIT: Can we give this a starting initial capacity so that it doesn't have to resize as we add things that we know will always be added?  I think it's fine if it's slightly larger than what's actually needed, so maybe assume the "worst" case where everything is added?  Not a huge deal tho.

> Source/WebCore/inspector/InspectorOverlay.cpp:1119
> +        labelContents.append({ layoutContextInfo.toString(), Color::black, InspectorOverlayLabel::Content::Decoration::Bordered, Color::gray.colorWithAlphaByte(64) });

I kinda feel like we should add this at the bottom of the tooltip, not the top.  IMO the selector and size of the element are *way* more important than whether the element is a grid container (which isn't to say that this isn't also important, cause it is).

> Source/WebCore/inspector/InspectorOverlay.cpp:1127
> +    labelContents.append({ makeString(emSpace), Color::black, InspectorOverlayLabel::Content::Decoration::FlexibleSpace });

IMO this looks kinda weird in the screenshot of the `h1` tooltip.  Perhaps it won't look so odd if it's in the top-right corner (see my comment on :1119) instead of in the middle 🤔

> Source/WebCore/inspector/InspectorOverlay.cpp:-1155
> -    constexpr auto elementTitleBackgroundColor = SRGBA<uint8_t> { 255, 255, 194 };

😱 Can you attach a screenshot of what this change would look like if you didn't change this color?  I'm not necessarily opposed, but i must admit there is some familiarity(/sentimentality) around this 😅

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:205
> +    bool startsNewLine = false;

Style: `{ false }`

Also, it seems like this is always overridden, so is it even necessary to provide a default?  I kinda feel like we want callers to explicitly give this a value.

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:206
> +    float computedWidth = 0;

ditto (:205) with `{ 0 }`

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:350
> +            float remainingLineContentWidth = 0;

How about `widthOfRemainingContent`?  IMO this isn't clear enough of a name to distinguish from `remainingLineWidth` 😅

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:358
> +        }
> +        case Content::Decoration::Bordered: {

Style: I'd add a newline in between these

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:359
> +            auto backgroundRect = FloatRoundedRect({ textPosition.x() + xOffset - labelContentDecorationBorderedLeadingAndTrailingPadding, textPosition.y() + ((line - 1) * lineHeight) + lineDescent, computedContentRun.computedWidth + (labelContentDecorationBorderedLeadingAndTrailingPadding * 2), lineHeight }, FloatRoundedRect::Radii(2));

NIT: Maybe inline this with some newlines in the `{` `}` so that each parameter gets its own space?

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:370
> +        }
> +        case Content::Decoration::None:

ditto (:357)

> Source/WebCore/inspector/InspectorOverlayLabel.h:91
> +        Decoration decoration = Decoration::None;
> +        Color decorationColor = Color::transparentBlack;

ditto (Source/WebCore/inspector/InspectorOverlayLabel.cpp:205)

NIT: also, should we make this into a
```
struct Decoration {
    enum class Type : uint8_t {
        ...
    };

    Type type;
    Color color;
};
```

[Patrick Angle]

Created attachment 452370 [details]
Patch v1.1 - Address review feedback, further design refinement

[Patrick Angle]

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

[Patrick Angle]

Created attachment 452467 [details]
Patch v1.2 - Remove label background/border changes

[Patrick Angle]

Created attachment 452468 [details]
Screenshot of Patch v1.2

[Devin Rousso]

Comment on attachment 452467 [details]
Patch v1.2 - Remove label background/border changes

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

r=me, neat stuff!

> Source/WebCore/inspector/InspectorOverlay.cpp:1093
> +    Vector<String> layoutContextBubbleStrings;
> +    if (rendererIsFlexboxItem(*renderer))

NIT: I'd add a newline between these since `layoutContextBubbleStrings` is used by more than just this `if`

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:349
> +                lineHeight

Style: Can we add a `,` at the end?

[Patrick Angle]

Created attachment 452476 [details]
Patch v1.3 - Review nits

[EWS]

Committed r290112 (247458@main): <https://commits.webkit.org/247458@main>

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