**Steps to repro:** 1. Navigate to https://labs.jensimmons.com/2017/03-008.html. 2. Enable the grid overlay for `main` in the Layout sidebar. 3. Resize page width until the content reflows to no longer use grid. 4. Resize page width back to original (or close to it). 5. Click the already-checked main checkbox. **Expected Result:** The checkbox would no longer be checked OR the overlay would be reenabled when it becomes a grid again. **Actual Result:** The checkbox is still checked without a grid overlay visible and unchecking the box results in: No grid overlay exists for the node, so cannot clear. (at Connection.js:162:29) _dispatchResponseToPromise @ Connection.js:162:29 _dispatchResponse @ Connection.js:124:44 dispatch @ Connection.js:77:35 dispatchMessageFromTarget @ TargetManager.js:176:39 dispatchMessageFromTarget @ TargetObserver.js:47:51 _dispatchEvent @ Connection.js:210:26 dispatch @ Connection.js:79:32 dispatch @ InspectorBackend.js:232:52 ? @ MessageDispatcher.js:42:34
<rdar://problem/74363006>
Created attachment 420462 [details] Patch
Comment on attachment 420462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420462&action=review > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:34 > + WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this); Rather than listening for every single layout context type change, can you just add this event listener inside `showGridOverlay` (and remove it in `hideGridOverlay`)? This way we don't have to run code for _every_ layout context type change (which is almost guaranteed going to be a far larger amount than just the handful (if even that many) of active overlays). This would also let you avoid having to check `this._gridOverlayForNodeMap.has(domNode)` in the event handler.
Created attachment 420503 [details] Patch
Comment on attachment 420462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420462&action=review >> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:34 >> + WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this); > > Rather than listening for every single layout context type change, can you just add this event listener inside `showGridOverlay` (and remove it in `hideGridOverlay`)? This way we don't have to run code for _every_ layout context type change (which is almost guaranteed going to be a far larger amount than just the handful (if even that many) of active overlays). This would also let you avoid having to check `this._gridOverlayForNodeMap.has(domNode)` in the event handler. That's a good idea! Uploaded a new patch implementing this.
Comment on attachment 420503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420503&action=review > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:102 > + if (domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid) { Style: we often prefer early-returns to avoid extra indentation Also, I think you should be able to assume that the new `layoutContextType` is not `WI.DOMNode.LayoutContextType.Grid` seeing as how the only way that `showGridOverlay` can get called is if the `domNode` is already a `WI.DOMNode.LayoutContextType.Grid` context. We should probably add `console.assert(domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid, domNode.layoutContextType);` inside `showGridOverlay` and `hideGridOverlay`. You also probably should remove it from `_gridOverlayForNodeMap`. ``` console.assert(domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid, domNode); this._gridOverlayForNodeMap.delete(domNode); domNode.removeEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this); this.dispatchEventToListeners(WI.OverlayManager.Event.GridOverlayHidden, this._gridOverlayForNodeMap.take(domNode)); ```
Comment on attachment 420503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420503&action=review >> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:102 >> + if (domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid) { > > Style: we often prefer early-returns to avoid extra indentation > > Also, I think you should be able to assume that the new `layoutContextType` is not `WI.DOMNode.LayoutContextType.Grid` seeing as how the only way that `showGridOverlay` can get called is if the `domNode` is already a `WI.DOMNode.LayoutContextType.Grid` context. We should probably add `console.assert(domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid, domNode.layoutContextType);` inside `showGridOverlay` and `hideGridOverlay`. > > You also probably should remove it from `_gridOverlayForNodeMap`. > ``` > console.assert(domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid, domNode); > this._gridOverlayForNodeMap.delete(domNode); > domNode.removeEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this); > this.dispatchEventToListeners(WI.OverlayManager.Event.GridOverlayHidden, this._gridOverlayForNodeMap.take(domNode)); > ``` > You also probably should remove it from `_gridOverlayForNodeMap`. That's what `this._gridOverlayForNodeMap.take(domNode)` is for.
Comment on attachment 420503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420503&action=review >>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:102 >>> + if (domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid) { >> >> Style: we often prefer early-returns to avoid extra indentation >> >> Also, I think you should be able to assume that the new `layoutContextType` is not `WI.DOMNode.LayoutContextType.Grid` seeing as how the only way that `showGridOverlay` can get called is if the `domNode` is already a `WI.DOMNode.LayoutContextType.Grid` context. We should probably add `console.assert(domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid, domNode.layoutContextType);` inside `showGridOverlay` and `hideGridOverlay`. >> >> You also probably should remove it from `_gridOverlayForNodeMap`. >> ``` >> console.assert(domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid, domNode); >> this._gridOverlayForNodeMap.delete(domNode); >> domNode.removeEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this); >> this.dispatchEventToListeners(WI.OverlayManager.Event.GridOverlayHidden, this._gridOverlayForNodeMap.take(domNode)); >> ``` > > Oh I totally missed that! Very clever move :) You might consider pulling that out into a variable tho just so that it's a bit more visible :P
Created attachment 420520 [details] Patch Carry over R+ from previous
(In reply to Razvan Caliman from comment #9) > Created attachment 420520 [details] > Patch > > Carry over R+ from previous Scratch that. No R+ to carry over. I got confused.
Comment on attachment 420520 [details] Patch (In reply to Razvan Caliman from comment #10) > (In reply to Razvan Caliman from comment #9) > > Created attachment 420520 [details] > > Patch > > > > Carry over R+ from previous > > Scratch that. No R+ to carry over. I got confused. I actually did r+ the previous patch, but I forgot to write "r=me". You can click the "History" link in the top-right next to "Modified:" and see for yourself :)
(In reply to Devin Rousso from comment #11) > Comment on attachment 420520 [details] > Patch > > (In reply to Razvan Caliman from comment #10) > > (In reply to Razvan Caliman from comment #9) > > > Created attachment 420520 [details] > > > Patch > > > > > > Carry over R+ from previous > > > > Scratch that. No R+ to carry over. I got confused. > I actually did r+ the previous patch, but I forgot to write "r=me". You can > click the "History" link in the top-right next to "Modified:" and see for > yourself :) Ha! Switching quickly between working on two patches. I knew I saw some R+ somewhere :) Thanks for the History tip!
Committed r272976: <https://commits.webkit.org/r272976> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420520 [details].