<rdar://87893171>
Created attachment 452264 [details] Patch v1.0
Created attachment 452265 [details] Screenshot of Patch v1.0
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; }; ```
Created attachment 452370 [details] Patch v1.1 - Address review feedback, further design refinement
Created attachment 452371 [details] Screenshot of Patch v1.1
Created attachment 452467 [details] Patch v1.2 - Remove label background/border changes
Created attachment 452468 [details] Screenshot of Patch v1.2
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?
Created attachment 452476 [details] Patch v1.3 - Review nits
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].