Show a list of all available flex containers in a separate section within the Layout sidebar. Similar approach to the one for listing CSS Grid containers in https://bugs.webkit.org/show_bug.cgi?id=221145
<rdar://87886241>
Created attachment 450063 [details] Patch 1.0 Implementation of Flexbox section in Layout sidebar. Extract generic functionality from the CSS Grid section and share between the two.
Comment on attachment 450063 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450063&action=review r-, please see my comment in `Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js` > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:26 > +.details-section:is(.layout-css-flexbox:not(.collapsed), .layout-css-grid:not(.collapsed)) > .content, Would this also work? ``` .details-section:is(.layout-css-flexbox, .layout-css-grid):not(.collapsed) > .content, ``` > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:161 > + break; > + case WI.DOMNode.LayoutContextType.Flex: Style: I'd add a newline in between these lines. > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:164 > + break; > + default: ditto (:160) > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:167 > + this._flexNodeList.delete(domNode); > + } Style: missing a `break;` > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:194 > + this._flexNodeList = new Set(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Flex)); Why are we suddenly calling these "List" even though they're actually `Set`? > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:2 > + * Copyright (C) 2021 Apple Inc. All rights reserved. 2022 > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:31 > +.node-overlay-list-section .node-display-name { Please put `>` in between selector components to be more specific (unless there's an anonymous (i.e. no CSS class/id/etc.) in between). ditto below > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:59 > + console.assert(typeof fn === "function", fn); > + this._showOverlay = fn; This is a really weird pattern. Normally we'd have something like ``` // Protected showOverlay() { // Implemented by subclasses. } ``` which the subclass would then override (possibly also calling `super.showOverlay()`). This would also let you avoid having to do any `bind` and can help make the internal JS object shape more consistent :) > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:137 > + // FIXME: Remove checks after implementing <https://webkit.org/b/235649> Toggle Flexbox Page Overlay from Layout sidebar > + if (this._showOverlay && this._hideOverlay) { So what exactly is the "Flexbox Overlays" section gonna look like right now? Can you attach a screenshot? I find it really odd that we're doing these patches in such incremental non-functional steps instead of fully complete features. It's more prone to things being reworked as new details are revealed during further implementation, resulting in greater code/git churn. Why can't we do it all at once?
Created attachment 450228 [details] Screenshot with patch applied Showing the list of flex containers available on the page with the "jump to" button to select the node in the DOM Tree view.
Comment on attachment 450063 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450063&action=review >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:59 >> + this._showOverlay = fn; > > This is a really weird pattern. Normally we'd have something like > ``` > // Protected > > showOverlay() > { > // Implemented by subclasses. > } > ``` > which the subclass would then override (possibly also calling `super.showOverlay()`). > > This would also let you avoid having to do any `bind` and can help make the internal JS object shape more consistent :) Yes, I can do that. I'll have make slight changes about checking for available functionality, like the ability to show/hide overlays, since we can no longer rely on the falsy value of the member. >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:137 >> + if (this._showOverlay && this._hideOverlay) { > > So what exactly is the "Flexbox Overlays" section gonna look like right now? Can you attach a screenshot? > > I find it really odd that we're doing these patches in such incremental non-functional steps instead of fully complete features. It's more prone to things being reworked as new details are revealed during further implementation, resulting in greater code/git churn. Why can't we do it all at once? > So what exactly is the "Flexbox Overlays" section gonna look like right now? Can you attach a screenshot? https://bug-235647-attachments.webkit.org/attachment.cgi?id=450228 It lists the flex containers found on the page. Each includes the "jump to" button to select to the node in the DOM Tree. > I find it really odd that we're doing these patches in such incremental non-functional steps instead of fully complete features. It's more prone to things being reworked as new details are revealed during further implementation, resulting in greater code/git churn. Why can't we do it all at once? As described in the changelog entry, the functionality and representation of flex containers in the Layout sidebar will be almost identical to the one already done for grid containers. Hence the code reuse here. The patches do provide incremental functionality. They are not just empty shells. The "fully complete feature" is predicated on a bunch of things. Implementing the specialized overlay is a blocker for much of the rest. Only then do the checkboxes to toggle visibility and color swatches make sense. The feature flag is an extra guard to shield STP users from the intermediate development state. Providing incremental patches should also ease both development and review. Why not do the work gradually?
Created attachment 450241 [details] Patch 1.1 Overwrite protected methods on subclasses to configure NodeOverlayListSection Introduce flags to suppress functionality in NodeOverlayListSection until necessary dependencies are implemented Address nits and code review feedback
Comment on attachment 450063 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450063&action=review >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:161 >> + case WI.DOMNode.LayoutContextType.Flex: > > Style: I'd add a newline in between these lines. Done >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:164 >> + default: > > ditto (:160) Done >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:167 >> + } > > Style: missing a `break;` Done >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:194 >> + this._flexNodeList = new Set(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Flex)); > > Why are we suddenly calling these "List" even though they're actually `Set`? No particular reason. I'll revert to Set. I vaguely remember we referred to Arrays and Sets as 'lists', but can't find a pattern in the code. >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:2 >> + * Copyright (C) 2021 Apple Inc. All rights reserved. > > 2022 Done >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:31 >> +.node-overlay-list-section .node-display-name { > > Please put `>` in between selector components to be more specific (unless there's an anonymous (i.e. no CSS class/id/etc.) in between). > > ditto below Most of the selectors on this page are for deeply nested nodes. I could add a few more members to increase specificity, but that quickly gets unyieldy and sensitive to any changes in DOM structure: ``` .node-overlay-list-section .node-display-name ``` becomes ``` .node-overlay-list-section > .node-overlay-list > li > .node-overlay-list-item-container > label > .node-display-name ``` or, skipping plain tag selectors: ``` .node-overlay-list-section > .node-overlay-list .node-overlay-list-item-container .node-display-name ``` This has always felt weird in WebInspector CSS. If the goal is to prevent specificity clashes, perhaps we can figure out a more durable solution (here, the `.node-overlay-list` is never used elsewhere outside the Layout panel) . Scoped stylesheets might be an idea. Should we try converting some DOM structures to Custom Elements with corresponding scoped stylesheets and exposed ::parts for host nodes to style on a case-by-case basis? I don't know exactly where the implementation is at in WebKit. I'd really want to learn if there are other ways to write this CSS without clobbering it with long selectors. CSS-in-JS isn't a crazy idea (though I'm not advocating using it here), but it does acknowledge that styles and markup are tightly coupled, especially in large, complex UIs like Web Inspector. As a developer, you always need to read the markup to understand the CSS. Co-locating them makes it easier to follow, modify and remove styles over time.
Comment on attachment 450063 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450063&action=review >>> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:31 >>> +.node-overlay-list-section .node-display-name { >> >> Please put `>` in between selector components to be more specific (unless there's an anonymous (i.e. no CSS class/id/etc.) in between). >> >> ditto below > > Most of the selectors on this page are for deeply nested nodes. > I could add a few more members to increase specificity, but that quickly gets unyieldy and sensitive to any changes in DOM structure: > > ``` > .node-overlay-list-section .node-display-name > ``` > becomes > ``` > .node-overlay-list-section > .node-overlay-list > li > .node-overlay-list-item-container > label > .node-display-name > ``` > or, skipping plain tag selectors: > ``` > .node-overlay-list-section > .node-overlay-list .node-overlay-list-item-container .node-display-name > ``` > > This has always felt weird in WebInspector CSS. If the goal is to prevent specificity clashes, perhaps we can figure out a more durable solution (here, the `.node-overlay-list` is never used elsewhere outside the Layout panel) > . > Scoped stylesheets might be an idea. Should we try converting some DOM structures to Custom Elements with corresponding scoped stylesheets and exposed ::parts for host nodes to style on a case-by-case basis? I don't know exactly where the implementation is at in WebKit. > > I'd really want to learn if there are other ways to write this CSS without clobbering it with long selectors. > > CSS-in-JS isn't a crazy idea (though I'm not advocating using it here), but it does acknowledge that styles and markup are tightly coupled, especially in large, complex UIs like Web Inspector. As a developer, you always need to read the markup to understand the CSS. Co-locating them makes it easier to follow, modify and remove styles over time. Our usual style is to be as explicit as possible where it makes sense. As an example, we usually don't list `div` or `span` as they're too generic, but things like `li` usually we do. The goal is both to prevent specificity clashes and to also maybe take advantage of the slightly more performant (at least last I heard) direct child selector as opposed to an any descendant selector. It also has the added benefit that if we do have a repeated selector in the same tree, the direct child selector will make it clear which one in the tree is being styled, not all of them (e.g. `.titles` is used by `WI.CanvasContentView`, and if we changed the implementation such that that element contained a `WI.GeneralTreeElement`, we probably wouldn't want a generic `.content-view.canvas .titles` (note that it does *not* have a direct child selector) to apply to both `.titles`). I don't think we want a rule of "never use a selector in more than one place" as there's lots of good reasons to re-use the same selector (e.g. `.titles`, `.group`, etc.). It also would require knowledge (or at least code searching skills) for the rest of Web Inspector, which I don't think is something we wanna require. I've experimented with using custom elements, but the annoyance there (at least when I did it, and I admit I could've done it "wrong") was that we had to create a `<style>` and somehow get the necessary/relevant text into it (which was made harder by the fact that we combine and minify our CSS (and JS) in production builds), and we had to do that for _every_ instance we created (tho I think there's a way to create a `<template>` that maybe would've avoided this?). I don't think we'd wanna do CSS-in-JS cause most of the styles we encounter never change, so there's not much benefit to having things be imperative (as opposed to the declarative method of having CSS files). >>> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:59 >>> + this._showOverlay = fn; >> >> This is a really weird pattern. Normally we'd have something like >> ``` >> // Protected >> >> showOverlay() >> { >> // Implemented by subclasses. >> } >> ``` >> which the subclass would then override (possibly also calling `super.showOverlay()`). >> >> This would also let you avoid having to do any `bind` and can help make the internal JS object shape more consistent :) > > Yes, I can do that. > > I'll have make slight changes about checking for available functionality, like the ability to show/hide overlays, since we can no longer rely on the falsy value of the member. I'd do that either with another `get canShowOverlay` that's overridden by the subclass or something like an `options = {}` in the `constructor` of `WI.NodeOverlayListSection`. >>> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:137 >>> + if (this._showOverlay && this._hideOverlay) { >> >> So what exactly is the "Flexbox Overlays" section gonna look like right now? Can you attach a screenshot? >> >> I find it really odd that we're doing these patches in such incremental non-functional steps instead of fully complete features. It's more prone to things being reworked as new details are revealed during further implementation, resulting in greater code/git churn. Why can't we do it all at once? > > > As described in the changelog entry, the functionality and representation of flex containers in the Layout sidebar will be almost identical to the one already done for grid containers. Hence the code reuse here. > > The patches do provide incremental functionality. They are not just empty shells. > > The "fully complete feature" is predicated on a bunch of things. Implementing the specialized overlay is a blocker for much of the rest. Only then do the checkboxes to toggle visibility and color swatches make sense. The feature flag is an extra guard to shield STP users from the intermediate development state. > > Providing incremental patches should also ease both development and review. > Why not do the work gradually? Perhaps "non-functional" was a bit too harsh. I apologize. Yes, strictly speaking there is new functionality here in that there is new stuff in the UI. But is it really *new* functionality? All that seems to be done in this patch is refactoring and exposing a list of `display: flex`, which frankly is very easy to find otherwise (and if the developer is the one who created the site being inspected, they likely have at least some idea of where the `display: flex` already are, so this list doesn't really give them anything new). I don't think this patch provides something that was not previously able to be done otherwise. To give a related example, the primary utility of the grid overlay is to visualize the CSS grid itself, not to list the `display: grid` nodes that exist on the page. And that feature was done incrementally, but each step contained a concrete *new* feature (e.g. grid lines, area names, etc.) that needed adoption in the UI. Generally speaking, we rarely have Web Inspector changes where the backend protocol and frontend adoption/usage are not part of the same patch, as it's more likely for things to get out-of-sync (i.e. some data returned by the protocol command is not actually needed). It also makes it harder to say "this feature is available after this revision/release" and can make rollouts/bisecting more challenging. Not to mention it requires that the reviewers of the code do their own research to figure out the parts being used, instead of having a more holistic view of everything at once. I would be much more OK with this change if it had at least something like one of the checkboxes/options present for the CSS grid feature because then it at least provides something new. I would also be OK with this patch just being the refactoring (but I am slightly hesitant even then as it'll likely involve assumptions being made about the future patches to come that necessitated the refactoring that may turn out to be false, resulting in even more changes). I can tell you as a reviewer that these kinds of incremental patches are *not* easier to review for the reasons above. I'm not saying that they're impossible (or even very difficult), but it requires me to ask more questions about "how do we expect this to be used" or "what is this eventually gonna look like" to be able to give good feedback, none of which I'd have to ask if I had the full (or at least a broader) picture. As an example, I have guesses as to what things we may wanna do with a CSS flex overlay (e.g. labeling the index of the child node, indicate the space between child nodes, something with wrapping, etc.), but I don't really know what exactly will be or how they'd fit in based on what you have in this patch.
Comment on attachment 450241 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=450241&action=review > Source/WebInspectorUI/UserInterface/Base/Setting.js:229 > + experimentalEnableFlexboxInspector: new WI.Setting("experimental-enable-flexbox-inspector", false), I think this should really be an engineering setting because right now it's not really usable. Normally, the word "experimental" implies that things are still a bit WIP, but they're also useable (just like STP, for example). I don't think that applies to this. It is still very much so engineering UI with large swathes of functional work still to be done, not something that has a few bugs in some edge cases. > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:32 > + this.sectionLabel = WI.UIString("Flexbox Overlays", "Page Overlays for Flex containers @ Layout Sidebar Section Header", "Heading for list of flex container nodes"); Again these could be a `get` that's overridden by this subclass instead of a `get`/`set` pair. See `get displayName` as an example of this pattern. > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:36 > + this.suppressOverlayColorChange = true Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:33 > + this.overlayShownEvent = WI.OverlayManager.Event.GridOverlayShown; > + this.overlayHiddenEvent = WI.OverlayManager.Event.GridOverlayHidden; The only reason to have these be `get`/`set` is if you expect them to be changed after they're set for the first time (and in that case you'd need to have logic to remove the listener for the old event and then add a listener for the new event). I don't think that's the intention, so these should also be `get overlayShownEvent` that's overridden by this subclass. > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:143 > + if (!this._flexNodeSet.size) { NIT: I'd flip the order of this so that we don't have to do the `!` and so that the "positive" scenario is first. > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:148 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:209 > + if (WI.settings.experimentalEnableFlexboxInspector) { Style: no `{` `}` with single line control flow > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:48 > + get overlayShownEvent() { return this._overlayShownEvent } Style: We only inline `get`/`set` if the implementation is simple and the corresponding `set`/`get` (if it exists) is also simple. Since the `set` is multi-line, please make this multi-line too.
Comment on attachment 450241 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=450241&action=review > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:109 > + if (!WI.settings.experimentalEnableFlexboxInspector) This should be `WI.settings.experimentalEnableFlexboxInspector.value`. > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:140 > + if (!WI.settings.experimentalEnableFlexboxInspector) Ditto (line 109). >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:209 >> + if (WI.settings.experimentalEnableFlexboxInspector) { > > Style: no `{` `}` with single line control flow Needs to end with `.value`.
Comment on attachment 450241 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=450241&action=review >> Source/WebInspectorUI/UserInterface/Base/Setting.js:229 >> + experimentalEnableFlexboxInspector: new WI.Setting("experimental-enable-flexbox-inspector", false), > > I think this should really be an engineering setting because right now it's not really usable. Normally, the word "experimental" implies that things are still a bit WIP, but they're also useable (just like STP, for example). I don't think that applies to this. It is still very much so engineering UI with large swathes of functional work still to be done, not something that has a few bugs in some edge cases. I agree with Devin. I added the engineering setting in a separate patch since my badges patch also depends on it now. bug 236091 Web Inspector: [Flexbox] Add setting to guard Flexbox Inspector feature
Created attachment 451435 [details] Patch 1.2 DEPENDS ON Bug 235924 - Adds protected methods and property getters to WI.NodeOverlayListSection and subclasses WI.CSSFlexboxSection and WI.CSSGridSection - Adds stricter CSS selectors to NodeOverlayListSection.css - Uses engineering setting as feature flag - Fixes nits - Updates localization files with new strings for empty messages
Comment on attachment 450063 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450063&action=review >>>> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:31 >>>> +.node-overlay-list-section .node-display-name { >>> >>> Please put `>` in between selector components to be more specific (unless there's an anonymous (i.e. no CSS class/id/etc.) in between). >>> >>> ditto below >> >> Most of the selectors on this page are for deeply nested nodes. >> I could add a few more members to increase specificity, but that quickly gets unyieldy and sensitive to any changes in DOM structure: >> >> ``` >> .node-overlay-list-section .node-display-name >> ``` >> becomes >> ``` >> .node-overlay-list-section > .node-overlay-list > li > .node-overlay-list-item-container > label > .node-display-name >> ``` >> or, skipping plain tag selectors: >> ``` >> .node-overlay-list-section > .node-overlay-list .node-overlay-list-item-container .node-display-name >> ``` >> >> This has always felt weird in WebInspector CSS. If the goal is to prevent specificity clashes, perhaps we can figure out a more durable solution (here, the `.node-overlay-list` is never used elsewhere outside the Layout panel) >> . >> Scoped stylesheets might be an idea. Should we try converting some DOM structures to Custom Elements with corresponding scoped stylesheets and exposed ::parts for host nodes to style on a case-by-case basis? I don't know exactly where the implementation is at in WebKit. >> >> I'd really want to learn if there are other ways to write this CSS without clobbering it with long selectors. >> >> CSS-in-JS isn't a crazy idea (though I'm not advocating using it here), but it does acknowledge that styles and markup are tightly coupled, especially in large, complex UIs like Web Inspector. As a developer, you always need to read the markup to understand the CSS. Co-locating them makes it easier to follow, modify and remove styles over time. > > Our usual style is to be as explicit as possible where it makes sense. As an example, we usually don't list `div` or `span` as they're too generic, but things like `li` usually we do. > > The goal is both to prevent specificity clashes and to also maybe take advantage of the slightly more performant (at least last I heard) direct child selector as opposed to an any descendant selector. It also has the added benefit that if we do have a repeated selector in the same tree, the direct child selector will make it clear which one in the tree is being styled, not all of them (e.g. `.titles` is used by `WI.CanvasContentView`, and if we changed the implementation such that that element contained a `WI.GeneralTreeElement`, we probably wouldn't want a generic `.content-view.canvas .titles` (note that it does *not* have a direct child selector) to apply to both `.titles`). > > I don't think we want a rule of "never use a selector in more than one place" as there's lots of good reasons to re-use the same selector (e.g. `.titles`, `.group`, etc.). It also would require knowledge (or at least code searching skills) for the rest of Web Inspector, which I don't think is something we wanna require. > > I've experimented with using custom elements, but the annoyance there (at least when I did it, and I admit I could've done it "wrong") was that we had to create a `<style>` and somehow get the necessary/relevant text into it (which was made harder by the fact that we combine and minify our CSS (and JS) in production builds), and we had to do that for _every_ instance we created (tho I think there's a way to create a `<template>` that maybe would've avoided this?). > > I don't think we'd wanna do CSS-in-JS cause most of the styles we encounter never change, so there's not much benefit to having things be imperative (as opposed to the declarative method of having CSS files). Done. Added more strict selectors using `>`. >>>> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:59 >>>> + this._showOverlay = fn; >>> >>> This is a really weird pattern. Normally we'd have something like >>> ``` >>> // Protected >>> >>> showOverlay() >>> { >>> // Implemented by subclasses. >>> } >>> ``` >>> which the subclass would then override (possibly also calling `super.showOverlay()`). >>> >>> This would also let you avoid having to do any `bind` and can help make the internal JS object shape more consistent :) >> >> Yes, I can do that. >> >> I'll have make slight changes about checking for available functionality, like the ability to show/hide overlays, since we can no longer rely on the falsy value of the member. > > I'd do that either with another `get canShowOverlay` that's overridden by the subclass or something like an `options = {}` in the `constructor` of `WI.NodeOverlayListSection`. No need for intermediary checks anymore. This patch depends on Bug 235924. Once that lands, this can follow.
Comment on attachment 450241 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=450241&action=review >> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:33 >> + this.overlayHiddenEvent = WI.OverlayManager.Event.GridOverlayHidden; > > The only reason to have these be `get`/`set` is if you expect them to be changed after they're set for the first time (and in that case you'd need to have logic to remove the listener for the old event and then add a listener for the new event). I don't think that's the intention, so these should also be `get overlayShownEvent` that's overridden by this subclass. Done. Only getters now expected to be implemented by subclasses >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:109 >> + if (!WI.settings.experimentalEnableFlexboxInspector) > > This should be `WI.settings.experimentalEnableFlexboxInspector.value`. Fixed. >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:140 >> + if (!WI.settings.experimentalEnableFlexboxInspector) > > Ditto (line 109). Fixed >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:143 >> + if (!this._flexNodeSet.size) { > > NIT: I'd flip the order of this so that we don't have to do the `!` and so that the "positive" scenario is first. Done >>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:209 >>> + if (WI.settings.experimentalEnableFlexboxInspector) { >> >> Style: no `{` `}` with single line control flow > > Needs to end with `.value`. Done and Fixed. >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:48 >> + get overlayShownEvent() { return this._overlayShownEvent } > > Style: We only inline `get`/`set` if the implementation is simple and the corresponding `set`/`get` (if it exists) is also simple. Since the `set` is multi-line, please make this multi-line too. No more setters.
Created attachment 451437 [details] Screenshot with patch applied
Comment on attachment 451435 [details] Patch 1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=451435&action=review > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:31 > + constructor() > + { > + super(); > + } You can omit this when there's no other logic besides the `super` call. > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:37 > + return WI.OverlayManager.Event.FlexOverlayShown; I think @Patrick Angle's comment <https://bugs.webkit.org/show_bug.cgi?id=235924#c6> might also apply here. > Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:30 > constructor() > { > super(); ditto (CSSFlexboxSection.js:28) > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:112 > + this._flexDetailsSectionRow = new WI.DetailsSectionRow(WI.UIString("No Flex Containers", "No Flex Containers @ Layout Details Sidebar Panel", "Message shown when there are no Flex containers on the inspected page.")); Should this be "CSS Flex" to match the "CSS Grid" above? > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:32 > + > + Style: only one newline
Comment on attachment 451435 [details] Patch 1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=451435&action=review > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:52 > + WI.overlayManager.showFlexOverlay(domNode, options); This hasn't been landed yet, right?
(In reply to Devin Rousso from comment #17) > Comment on attachment 451435 [details] > Patch 1.2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451435&action=review > > > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:52 > > + WI.overlayManager.showFlexOverlay(domNode, options); > > This hasn't been landed yet, right? It has not, I'm working on it now in Bug 235924
Comment on attachment 451435 [details] Patch 1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=451435&action=review >> Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:31 >> + } > > You can omit this when there's no other logic besides the `super` call. Done >> Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:37 >> + return WI.OverlayManager.Event.FlexOverlayShown; > > I think @Patrick Angle's comment <https://bugs.webkit.org/show_bug.cgi?id=235924#c6> might also apply here. Refactored and inlined event names and methods from implemented in `OverlayManager` in Bug 235924. Removed protected methods. >> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:30 >> super(); > > ditto (CSSFlexboxSection.js:28) Done >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:112 >> + this._flexDetailsSectionRow = new WI.DetailsSectionRow(WI.UIString("No Flex Containers", "No Flex Containers @ Layout Details Sidebar Panel", "Message shown when there are no Flex containers on the inspected page.")); > > Should this be "CSS Flex" to match the "CSS Grid" above? Done >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:32 >> + > > Style: only one newline Done
Created attachment 451596 [details] Patch 1.3 Inlined event names and direct method calls to OverlayManager Addressed nits
(In reply to Razvan Caliman from comment #20) > Created attachment 451596 [details] > Patch 1.3 DEPENDS ON BUG 235924
*** Bug 235650 has been marked as a duplicate of this bug. ***
*** Bug 235649 has been marked as a duplicate of this bug. ***
Created attachment 451668 [details] Patch 1.4 Patch rebased on tip-of-tree; no longer depends on bug 235924
Comment on attachment 451668 [details] Patch 1.4 View in context: https://bugs.webkit.org/attachment.cgi?id=451668&action=review Nice! Just some thoughts on naming and a request in `LayoutDetailsSidebarPanel.prototype._handleLayoutContextTypeChanged` so that we hopefully don't have to refactor it /again/ some day 😅. > Source/WebInspectorUI/UserInterface/Main.html:658 > <script src="Views/CSSStyleSheetTreeElement.js"></script> > <script src="Views/CSSGridSection.js"></script> > + <script src="Views/CSSFlexboxSection.js"></script> Nit: We seem to have drifted from being in alphabetical order here at `Views/CSS*`. Could we get these three tags back in order since we are adding one of them? > Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:26 > +WI.CSSFlexboxSection = class CSSFlexboxSection extends WI.NodeOverlayListSection IMO it would make the relationship between the subclass and superclass clearer at its usage if this was named `FlexboxNodeOverlayListSection` or similar. The current name is /fine/ but given there is also the grid section, it would be nice for them to share a bit more in their name, since they share almost all their implementation. > Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:26 > +WI.CSSGridSection = class CSSGridSection extends WI.NodeOverlayListSection See CSSFlexboxSection.js:26 > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:181 > + // A node may switch layout context type between grid and flex. > + // Remove it from the opposite node set in case it was already collected. > + switch (domNode.layoutContextType) { > + case WI.DOMNode.LayoutContextType.Grid: > this._gridNodeSet.add(domNode); > - else > + this._flexNodeSet.delete(domNode); > + break; > + > + case WI.DOMNode.LayoutContextType.Flex: > + this._flexNodeSet.add(domNode); > + this._gridNodeSet.delete(domNode); > + break; > + > + default: > this._gridNodeSet.delete(domNode); > + this._flexNodeSet.delete(domNode); > + break; > + } I wonder if instead it would make sense to have DOMNode.prototype.layoutContextType set provide the previous mode as part of its event data, and then have two switch/cases that removes the node from its previous location as determined by its previous layout context, and another that only adds it back to the new correct place? Or, given how generally unlikely it is that a node changes layout context type in general, we could just start by removing the node from both sets and then adding it to the appropriate set inside the switch/case. Much like my comments the other day on Nikita's patch implementing the badges and OverlayManager refactoring for this, I am weary of unscalable approaches if another context were to become instrumented and had an overlay in the future, given how much we already have to adjust this time around to add the second overlay.
Comment on attachment 451668 [details] Patch 1.4 View in context: https://bugs.webkit.org/attachment.cgi?id=451668&action=review >> Source/WebInspectorUI/UserInterface/Main.html:658 >> + <script src="Views/CSSFlexboxSection.js"></script> > > Nit: We seem to have drifted from being in alphabetical order here at `Views/CSS*`. Could we get these three tags back in order since we are adding one of them? Done >> Source/WebInspectorUI/UserInterface/Views/CSSFlexboxSection.js:26 >> +WI.CSSFlexboxSection = class CSSFlexboxSection extends WI.NodeOverlayListSection > > IMO it would make the relationship between the subclass and superclass clearer at its usage if this was named `FlexboxNodeOverlayListSection` or similar. The current name is /fine/ but given there is also the grid section, it would be nice for them to share a bit more in their name, since they share almost all their implementation. That's a good suggestion. It gets wordy, but I didn't have a shorter, clearer name for `NodeOverlayListSection`. On the plus side, This gives me a chance to rename as `CSSFlexNodeOverlayListSection` and use the Flex shorthand we used elsewhere for this feature (ex: `DOMAgent.showFlexOverlay()`). For consistency. >> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:26 >> +WI.CSSGridSection = class CSSGridSection extends WI.NodeOverlayListSection > > See CSSFlexboxSection.js:26 Renamed to `CSSGridNodeOverlayListSection`. >> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:181 >> + } > > I wonder if instead it would make sense to have DOMNode.prototype.layoutContextType set provide the previous mode as part of its event data, and then have two switch/cases that removes the node from its previous location as determined by its previous layout context, and another that only adds it back to the new correct place? > Or, given how generally unlikely it is that a node changes layout context type in general, we could just start by removing the node from both sets and then adding it to the appropriate set inside the switch/case. Much like my comments the other day on Nikita's patch implementing the badges and OverlayManager refactoring for this, I am weary of unscalable approaches if another context were to become instrumented and had an overlay in the future, given how much we already have to adjust this time around to add the second overlay. If the goal is to be prepared to scale for other context types, I think your second suggestion works better: - it co-locates the logic to remove the node from any list it may be in. - it doesn't require knowledge about the previous context type. Yes, changing from one layout context to the other (flex to grid) is likely to be rare.
Created attachment 451910 [details] Patch 1.5 - Rename to WI.CSSGridNodeOverlayListSection and CSSFlexNodeOverlayListSection.js - Address nits
Comment on attachment 451910 [details] Patch 1.5 View in context: https://bugs.webkit.org/attachment.cgi?id=451910&action=review r=me > Source/WebInspectorUI/ChangeLog:83 > +2022-02-11 Razvan Caliman <rcaliman@apple.com> Oops: Double changelog entry > Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:115 > + if (domNode.layoutContextType == WI.DOMNode.LayoutContextType.Grid) Nit: `===`
Comment on attachment 451910 [details] Patch 1.5 View in context: https://bugs.webkit.org/attachment.cgi?id=451910&action=review >> Source/WebInspectorUI/ChangeLog:83 >> +2022-02-11 Razvan Caliman <rcaliman@apple.com> > > Oops: Double changelog entry Doh! Changelog all the things! Removed. >> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:115 >> + if (domNode.layoutContextType == WI.DOMNode.LayoutContextType.Grid) > > Nit: `===` Good catch!
Created attachment 451919 [details] Patch 1.6 Address final nits
Comment on attachment 451919 [details] Patch 1.6 Carry over R+
ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.
Created attachment 451930 [details] Patch 1.6 Carry over R+; Fix Changelog
Committed r289757 (247230@main): <https://commits.webkit.org/247230@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451930 [details].