Show "flex" badge for elements with `display: flex` and `display: inline-flex`. <rdar://problem/87886003>
Created attachment 450746 [details] WIP NOT ready for review. This adds "Engineering -> Enable flexbox inspector" checkbox. Once enabled, it displays "flex" badges. Because this patch depends on bug 236013, Clicking on "flex" badges currently throws an exception: undefined is not an object (evaluating 'target.DOMAgent.showFlexOverlay.invoke') (at OverlayManager.js:116:40)
Comment on attachment 450746 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=450746&action=review > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:253 > + this._nextDefaultGridColorIndex = (this._nextDefaultFlexColorIndex + 1) % WI.OverlayManager.defaultHSLColors.length; We can make an infinitely looping iterator from the colors array and remove the all the associated logic for managing indexes. Context: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_iterator_protocol Perhaps this helper can live in `Source/WebInspectorUI/UserInterface/Base/Utilities.js`: ``` function makeInfiniteLoopingIterable(iterable) { console.assert(typeof iterable[Symbol.iterator] === "function", "Object is not iterable"); let _iterator = iterable[Symbol.iterator](); return { next: function() { let result = _iterator.next(); if (result.done) { this.reset(); return this.next(); } return result; }, reset: function() { _iterator = iterable[Symbol.iterator](); }, [Symbol.iterator]: function() { return this; } } } ``` It can be used like this: ``` // Create separate iterators for each overlay type this._defaultGridColorsIterator = makeInfiniteLoopingIterable(WI.OverlayManager.defaultHSLColors); this._defaultFlexColorsIterator = makeInfiniteLoopingIterable(WI.OverlayManager.defaultHSLColors); // ... // Keep calling .next() to the next color. Will loop automatically to the beginning when it reaches the end. defaultColor = this._defaultGridColorsIterator.next().value; // Manually reset the iterator to the first position, for example in `_handleMainResourceDidChange()` this._defaultGridColorsIterator.reset() ```
Created attachment 451344 [details] Patch Fixed the bug when all flexbox overlays used the same pink color. The patch has a lot of straightforward changes to OverlayManager.js. Most grid-related methods and variables now have their flex counterparts, e.g. showGridOverlay/showFlexOverlay. This doesn't change the existing grid logic, which is good. The downside is that now we have a fair amount of code duplication. It's tempting to refactor this, but I don't think it's a good idea for the next 7 days.
(In reply to Razvan Caliman from comment #2) Hm, I'm not strongly opposed to this, but I'm trying to minimize amount of changes (such as refactoring) in this patch.
Comment on attachment 451344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451344&action=review I'm not convinced that what you've done in OverlayManager is the right approach. Other than the actual command arguments and underlying protocol command, all the logic is shared between flex/grid overlays. Why do we need separate maps of nodes-to-color for the visible overlays? Why have separate events for grid/flex overlays when the event includes the overlay which has a node that tells you if it was/is a grid/flex container? I understand some things like getting the default colors need separate buckets to make sure that each set is a clean, repeating, sequence of colors, but a lot of other logic in here appears to just be duplicated between flex/grid which will only get more annoying if more overlays were to exist some day. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2024 > + let isGrid = this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Grid; > + let isFlex = this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Flex && WI.settings.engineeringEnableFlexboxInspector.value; We should probably just do a switch-case below where we set the text. And then here do a check for no layout context type (null) and iff the layout context is Flex then check the setting as well. E.g. ``` if (!this.representedObject.layoutContextType) return; if (this.representedObject.layoutContextType = WI.DOMNode.LayoutContextType.Flex && !WI.settings.engineeringEnableFlexboxInspector.value) return; ``` which makes it easier to scale with future overlays. And then use switch-case below for :2031-2034 with an assertion that we don't hit the `default` case. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2087 > if (domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid) { > - WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayShown, this._updateGridBadgeStatus, this); > - WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._updateGridBadgeStatus, this); > + WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayShown, this._updateLayoutBadgeStatus, this); > + WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._updateLayoutBadgeStatus, this); > } else { > - WI.overlayManager.removeEventListener(WI.OverlayManager.Event.GridOverlayShown, this._updateGridBadgeStatus, this); > - WI.overlayManager.removeEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._updateGridBadgeStatus, this); > + WI.overlayManager.removeEventListener(WI.OverlayManager.Event.GridOverlayShown, this._updateLayoutBadgeStatus, this); > + WI.overlayManager.removeEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._updateLayoutBadgeStatus, this); > } This doesn't seem correct. Don't we need to update for flex as well?
(In reply to Patrick Angle from comment #6) > Comment on attachment 451344 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451344&action=review > > I'm not convinced that what you've done in OverlayManager is the right > approach. Other than the actual command arguments and underlying protocol > command, all the logic is shared between flex/grid overlays. Why do we need > separate maps of nodes-to-color for the visible overlays? Why have separate > events for grid/flex overlays when the event includes the overlay which has > a node that tells you if it was/is a grid/flex container? I understand some > things like getting the default colors need separate buckets to make sure > that each set is a clean, repeating, sequence of colors, but a lot of other > logic in here appears to just be duplicated between flex/grid which will > only get more annoying if more overlays were to exist some day. I think you might be right. I'm going to try to reduce duplication now.
Created attachment 451462 [details] Patch
Comment on attachment 451462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451462&action=review > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:56 > + console.assert(domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid || domNode.layoutContextType === WI.DOMNode.LayoutContextType.Flex, domNode.layoutContextType); NIT: Is there any reason to enumerate each of the `WI.DOMNode.LayoutContextType`? Or can we just `Object.values(WI.DOMNode.LayoutContextType).includes(domNode.layoutContextType)`? > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:72 > + break; > + case WI.DOMNode.LayoutContextType.Flex: NIT: I'd add a newline between these > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:-85 > - console.assert(domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid, domNode.layoutContextType); Why is this removed? We could have this instead of the `default:` below. > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106 > + break; > + case WI.DOMNode.LayoutContextType.Flex: ditto (:71) > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:111 > + default: > + console.assert(false, domNode.layoutContextType); > + } missing a `break;` > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:126 > + isOverlayVisible(domNode) NIT: Maybe `hasVisibleOverlay` instead? > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:153 > + break; > + case WI.DOMNode.LayoutContextType.Flex: ditto (:71) > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:188 > + console.assert(domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid && domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Flex, domNode); ditto (:56) > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:222 > +WI.OverlayManager.defaultHSLColors = [ NIT: this should probably be `WI.OverlayManager._defaultHSLColors` since it's not meant to be used externally > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2030 > + this._layoutBadgeElement.className = "badge"; please make this `layout-badge` to better match the variable name (and be less generic) > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2036 > + break; > + case WI.DOMNode.LayoutContextType.Flex: ditto (Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:71) > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2041 > + default: > + console.assert(false, this.representedObject.layoutContextType); > + } ditto (Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:109) > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2044 > + this.title.append(this._layoutBadgeElement); NIT: you can(/should) combine this with :2029 ``` this._layoutBadgeElement = this.title.appendChild(document.createElement("span")); ``` > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2058 > + let initiator = this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Grid ? WI.GridOverlayDiagnosticEventRecorder.Initiator.Badge : null; NIT: I'd either inline this ``` WI.overlayManager.toggleOverlay(this.representedObject, { initiator: this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Grid ? WI.GridOverlayDiagnosticEventRecorder.Initiator.Badge : null, }); ``` or make it so that you don't even create the property unless it has a value ``` let options = {}; if (this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Grid) options.initiator = WI.GridOverlayDiagnosticEventRecorder.Initiator.Badge; WI.overlayManager.toggleOverlay(this.representedObject, options); ```
Created attachment 451603 [details] Patch
Comment on attachment 451603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451603&action=review r=me, nice work! I'm glad we were able to do this with a lot less duplicated code. > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:190 > + console.assert(Object.values(WI.DOMNode.LayoutContextType).includes(domNode.layoutContextType), domNode); I don't think this assertion will pass if a node becomes something other than the currently recognized context types of Grid and Flex. Because changing from one context type to any other context type, or null, is valid I don't think we need an assertion here any more. > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:220 > this._nextDefaultGridColorIndex = 0; > + this._nextDefaultFlexColorIndex = 0; As an aside, I think we might have a pre-existing bug where the order of colors is possibly dependent on if the Layout sidebar panel is open when loading a page because only when it is open are all the various layout containers sent to the frontend. If just the DOM tree is visible, only the DOM nodes that have made it from the backend to the frontend will get badges, which means deeply nested child nodes with grid or flex contexts aren't known about until the tree is expanded, at which point you might have used color 1 before the child somewhere and color 2 after the child somewhere, which means we might use color 3 for the child, which in the document is declared between the first and second nodes that happened to already be visible. In practice I don't think this is too big of a deal, and given it is an existing potential issue with mild symptoms we certainly don't need to solve it in this patch. But I thought I'd mention it while it was fresh in my mind.
Created attachment 451650 [details] Patch
Committed r289610 (247123@main): <https://commits.webkit.org/247123@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451650 [details].