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
Created attachment 453234 [details] [fast-cq] Patch
Created attachment 453236 [details] [Image] after Patch is applied (Page Overlay)
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 453249 [details] [Image] after Patch is applied (Layout panel)
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.
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.
Comment on attachment 453234 [details] [fast-cq] Patch r=me based on our discussion - thank you for clarifying!
Created attachment 453410 [details] [fast-cq] Patch
ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.
Created attachment 453419 [details] [fast-cq] Patch
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].
<rdar://problem/89577732>