Bug 221919 - Web Inspector: Grids with overlays that disappear and reappear remain checked in Layout pane, toggling hits assertion
Summary: Web Inspector: Grids with overlays that disappear and reappear remain checked...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-15 13:23 PST by Patrick Angle
Modified: 2021-02-16 19:47 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.81 KB, patch)
2021-02-16 06:11 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2021-02-16 11:16 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (3.79 KB, patch)
2021-02-16 12:23 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-02-15 13:23:38 PST
**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
Comment 1 Radar WebKit Bug Importer 2021-02-15 13:24:06 PST
<rdar://problem/74363006>
Comment 2 Razvan Caliman 2021-02-16 06:11:23 PST
Created attachment 420462 [details]
Patch
Comment 3 Devin Rousso 2021-02-16 09:36:48 PST
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.
Comment 4 Razvan Caliman 2021-02-16 11:16:11 PST
Created attachment 420503 [details]
Patch
Comment 5 Razvan Caliman 2021-02-16 11:16:51 PST
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 6 Devin Rousso 2021-02-16 12:00:32 PST
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 7 Razvan Caliman 2021-02-16 12:10:53 PST
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 8 Devin Rousso 2021-02-16 12:12:45 PST
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
Comment 9 Razvan Caliman 2021-02-16 12:23:49 PST
Created attachment 420520 [details]
Patch

Carry over R+ from previous
Comment 10 Razvan Caliman 2021-02-16 12:25:03 PST
(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 11 Devin Rousso 2021-02-16 12:26:55 PST
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 :)
Comment 12 Razvan Caliman 2021-02-16 12:28:43 PST
(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!
Comment 13 EWS 2021-02-16 19:47:22 PST
Committed r272976: <https://commits.webkit.org/r272976>

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