Bug 237215 – Web Inspector: [Flexbox] Add options to show each area's CSS `order` and/or DOM index in the parent flex container

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

[Devin Rousso]

this will help developers better understand how CSS `order` and the DOM index of each flex item interact and result in what's eventually rendered

[Devin Rousso]

Created attachment 453234 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 453236 [details]
[Image] after Patch is applied (Page Overlay)

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 453249 [details]
[Image] after Patch is applied (Layout panel)

[Patrick Angle]

Comment on attachment 453234 [details]
[fast-cq] Patch

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

Looks good - just the one concern re: child indexes and how they are counted

> Source/WebCore/inspector/InspectorOverlay.cpp:1896
> +                if (renderChildrenInFlexOrder.contains(renderer))

It seems odd to me that we wouldn't count children of the flex container that happen to not be participating in the flex layout - it kinda messes with the meaning of `Child #`. For example, it would make more sense, IMO, to still count non-participating children so that the child number corresponds with the node more closely, so if I'm counting to the third child in the DOM tree I don't need to remember that I'm actually looking for the 4th child because a previous sibling to a child isn't participating in the flex layout.

> Source/WebCore/inspector/InspectorOverlay.cpp:1919
> +                Vector<InspectorOverlayLabel::Content> orderNumbers;
> +                auto addOrderNumber = [&] (String&& number) {
> +                    if (!orderNumbers.isEmpty())
> +                        orderNumbers.append({ "\n"_s, Color::black });
> +                    orderNumbers.append({ WTFMove(number), Color::black });
> +                };

Nit: Because all the `Content`s have the same styling, we could just use a StringBuilder to construct a single string. InspectorOverlayLabel will handle splitting the lines even when `\n` is in middle of other text.

> Source/WebCore/inspector/InspectorOverlay.cpp:1925
> +                if (auto order = renderChild->style().order())
> +                    addOrderNumber(makeString("order: ", order));

Is there a way we could show `order: 0` when other children has a non-zero order value? It might look cleaner for all the items to have the same pieces of information, as well as prevent the "wait, why doesn't this one tell me the order but the other ones do" confusion before someone realizes that we are dropping all `order: 0`s, even when other children may have an `order` other than zero.

[Devin Rousso]

Comment on attachment 453234 [details]
[fast-cq] Patch

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:1896
>> +                if (renderChildrenInFlexOrder.contains(renderer))
> 
> It seems odd to me that we wouldn't count children of the flex container that happen to not be participating in the flex layout - it kinda messes with the meaning of `Child #`. For example, it would make more sense, IMO, to still count non-participating children so that the child number corresponds with the node more closely, so if I'm counting to the third child in the DOM tree I don't need to remember that I'm actually looking for the 4th child because a previous sibling to a child isn't participating in the flex layout.

My thinking here is that I was really just trying to visualize "what's the DOM order of the visible flex items", not "what is the index of this flex child in the `childNodes` of the flex container".  I found that doing the latter confused me because I would think "wait why is there no Child #2 where is it???" (and it would be even worse if we considered non-visible children, e.g. whitespace).

As we discussed offline, I think just changing this to "Item #" will immediately clarify that intention (and will match the "Flex Item" that's now shown in the element tooltip).

>> Source/WebCore/inspector/InspectorOverlay.cpp:1919
>> +                };
> 
> Nit: Because all the `Content`s have the same styling, we could just use a StringBuilder to construct a single string. InspectorOverlayLabel will handle splitting the lines even when `\n` is in middle of other text.

Yeah I could do that.  This was kinda leftover from an old structure, so I'll adjust :)

>> Source/WebCore/inspector/InspectorOverlay.cpp:1925
>> +                    addOrderNumber(makeString("order: ", order));
> 
> Is there a way we could show `order: 0` when other children has a non-zero order value? It might look cleaner for all the items to have the same pieces of information, as well as prevent the "wait, why doesn't this one tell me the order but the other ones do" confusion before someone realizes that we are dropping all `order: 0`s, even when other children may have an `order` other than zero.

Good idea.  Will change.

[Patrick Angle]

Comment on attachment 453234 [details]
[fast-cq] Patch

r=me based on our discussion - thank you for clarifying!

[Devin Rousso]

Created attachment 453410 [details]
[fast-cq] Patch

[EWS]

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

[Devin Rousso]

Created attachment 453419 [details]
[fast-cq] Patch

[EWS]

Committed r290613 (247886@main): <https://commits.webkit.org/247886@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89577732>