Bug 202065 – REGRESSION(r243264): Web Inspector: Style pane doesn't update after toggling CSS class

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 379288 [details]
Reduction 1

Steps:
1. Open toggle-class.html
2. Inspect <div class="a b">
3. Click "Classes" button at the bottom right corner of Web Inspector
4. Unckeck "b" class

Expected:
The style editor should not show "b" style rule anymore.

Actual:
The style editor doesn't update — "b" style rule is still visible.

Notes:
Regressed in bug 178489: Web Inspector: Styles Redesign: Editing selector should not hide the rule
https://trac.webkit.org/changeset/243264/webkit

Interestingly, editing "class" attribute in the DOM outline works fine.

[Nikita Vasilyev]

<rdar://problem/55149141>

[Nikita Vasilyev]

r243264 removed code that cleared the maps. After r243264, maps don't get cleared.

         function fetchedMatchedStyles(error, matchedRulesPayload, pseudoElementRulesPayload, inheritedRulesPayload)
         {
@@ -142,20 +188,12 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
             pseudoElementRulesPayload = pseudoElementRulesPayload || [];
             inheritedRulesPayload = inheritedRulesPayload || [];
 
-            // Move the current maps to previous.
-            this._previousRulesMap = this._rulesMap;
-            this._previousStyleDeclarationsMap = this._styleDeclarationsMap;
-
-            // Clear the current maps.
-            this._rulesMap = {};
-            this._styleDeclarationsMap = {};

Because of this, WI.DOMNodeStyles.Event.Refreshed gets called with significantChange set to false. significantChange can be true only when a style rule is new (and not present in previousStylesMap).

The style rules only get updated when WI.DOMNodeStyles.Event.Refreshed is fired with significantChange set to true.

[Nikita Vasilyev]

Created attachment 379298 [details]
Patch

This fixes the bug and clears the maps as it was before the regression was introduced.

However, I've noticed that the existing (and pretty old) code may have race conditions. `fetchedMatchedStyles` and` fetchedComputedStyle` are called in parallel and they modify the same variables. I'll take a closer look.

[Joseph Pecoraro]

> However, I've noticed that the existing (and pretty old) code may have race
> conditions. `fetchedMatchedStyles` and` fetchedComputedStyle` are called in
> parallel and they modify the same variables. I'll take a closer look.

Backend commands, unless they are marked `async`, are serial. So the order of responses should be deterministic.

[Nikita Vasilyev]

Comment on attachment 379298 [details]
Patch

Oh, the patch should be good to review then!

[Nikita Vasilyev]

Comment on attachment 379298 [details]
Patch

Several tests are failing :(

[Nikita Vasilyev]

Created attachment 379419 [details]
Patch

To run on bots. Not for review.

[Nikita Vasilyev]

Comment on attachment 379419 [details]
Patch

Setting r? so the bots run the tests. Don't review.

[Nikita Vasilyev]

Created attachment 379420 [details]
Patch for bots

[Devin Rousso]

Comment on attachment 379420 [details]
Patch for bots

View in context: https://bugs.webkit.org/attachment.cgi?id=379420&action=review

r-, we don't want to just revert these changes, as IIRC they were necessary in order for r243264 to work correctly.

(In reply to Nikita Vasilyev from comment #2)
> r243264 removed code that cleared the maps. After r243264, maps don't get cleared.
> 
>          function fetchedMatchedStyles(error, matchedRulesPayload, pseudoElementRulesPayload, inheritedRulesPayload)
>          {
> @@ -142,20 +188,12 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>              pseudoElementRulesPayload = pseudoElementRulesPayload || [];
>              inheritedRulesPayload = inheritedRulesPayload || [];
>  
> -            // Move the current maps to previous.
> -            this._previousRulesMap = this._rulesMap;
> -            this._previousStyleDeclarationsMap = this._styleDeclarationsMap;
> -
> -            // Clear the current maps.
> -            this._rulesMap = {};
> -            this._styleDeclarationsMap = {};
> 
> Because of this, WI.DOMNodeStyles.Event.Refreshed gets called with significantChange set to false. significantChange can be true only when a style rule is new (and not present in previousStylesMap).
> 
> The style rules only get updated when WI.DOMNodeStyles.Event.Refreshed is fired with significantChange set to true.
If that's the case, can we change the logic for how `significantChange` is determined instead?  It seems like that is the real issue here, not how (or what) we decide to keep in a map.

> Source/WebInspectorUI/ChangeLog:9
> +        Revert all changes related to `_styleDeclarationsMap`.

Why does this solve the problem?  Please add some description either to your investigation or why/how you arrived at this conclusion in the ChangeLog.

[Nikita Vasilyev]

This wasn’t ready for review. Sorry it wasn’t obvious. Bots don’t run tests if I set “r-“ :/

[Nikita Vasilyev]

Created attachment 379579 [details]
Patch for bots, not for review

[Nikita Vasilyev]

Created attachment 379611 [details]
Patch

[Nikita Vasilyev]

Created attachment 379612 [details]
[Animated GIF] With patch applied

[Devin Rousso]

Comment on attachment 379611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379611&action=review

This causes a UI regression the Styles panel:
1. inspect <https://devinrousso.com>
2. select the node matching `body > header > nav > ul > li:nth-child(1) > a` in the Elements Tab
3. change the `a.roll` selector to be some garbage (e.g. "asdasdasd")
4. change the `header>nav>ul>li>a.selected` selector to be some garbage (e.g. "zxczxczxc")
 => all the group "headers" (e.g. "Pseudo-Element ..." or "Inherited From ...") move to the bottom of the styles sidebar

I think this has something to do with the fact that `_headerMap` (`WI.SpreadsheetRulesStyleDetailsPanel`) uses the specific `WI.CSSStyleDeclaration` instance as a key to reference the DOM node group "header".

> Source/WebInspectorUI/ChangeLog:9
> +        r243264 never cleared `_styleMap`. Create and clear `_styleMap` at the same place as it was before r243264.

Why does clearing the `_styleMap` fix this issue?  Can you explain what's going on here?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:-161
> -        let previousStylesMap = this._stylesMap.copy();

I think this was the only usage of `Multimap.prototype.copy`.  If it's this expensive, perhaps we should remove it?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #15)
> Comment on attachment 379611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379611&action=review
> 
> This causes a UI regression the Styles panel: [...]

Yes, I can reproduce it.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #15)
> Comment on attachment 379611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379611&action=review
> 
> This causes a UI regression the Styles panel:
> 1. inspect <https://devinrousso.com>
> 2. select the node matching `body > header > nav > ul > li:nth-child(1) > a`
> in the Elements Tab
> 3. change the `a.roll` selector to be some garbage (e.g. "asdasdasd")
> 4. change the `header>nav>ul>li>a.selected` selector to be some garbage
> (e.g. "zxczxczxc")
>  => all the group "headers" (e.g. "Pseudo-Element ..." or "Inherited From
> ...") move to the bottom of the styles sidebar

This causes the UI regression because it fixed the significantChange logic.

r243264 introduced a regression in the significantChange logic, so WI.SpreadsheetRulesStyleDetailsPanel#layout wouldn't get called when it should. That was the real reason non-matching rules stayed visible.

This code was added in r243264:


    layout()
    {
        ...
        if (preservedSections.length) {
            for (let section of oldSections) {
                if (!preservedSections.includes(section))
                    this.removeSubview(section);
            }
            for (let header of this._headerMap.values())
                header.remove();
        }

It's a dead code. If you go through the steps on ToT WebKit, you can see that WI.SpreadsheetRulesStyleDetailsPanel#layout doesn't even get called on step 3 or 4.

When I reverted significantChange logic back to normal, this code started running.

[Devin Rousso]

Comment on attachment 379611 [details]
Patch

r-, since this causes a regression.

[Nikita Vasilyev]

Created attachment 407500 [details]
Reduction 2

This also reproduces when JS on a web page changes CSS classes related to the inspected element. (via rdar://67419397)

Steps:
1. Open "Reduction 2"
2. Inspect <body>
3. Press "Toggle background" button on the web page

Expected:
The Styles sidebar should no longer show ".foo" rule.

[Nikita Vasilyev]

There are two distinct use cases in play here:

1. CSS class added/removed from an element causing the style editor to update. The style editor should update with CSSAgent.getMatchedStylesForNode payload. (Currently broken.)

2. Web developer modified CSS selector in the style editor. The style editor should NOT update — the modified CSS rule should stay visible.

Pretty straightforward so far.

What happens if the 2nd case is followed by the 1st? This is where it may get complicated. Two different solutions:

EASY
Show only matching rules. That’s what both Chrome DevTools and Firefox DevTools do.

HARD
Show both matching rules and non-matching rules. If we do this, how do we reconcile matching and non-matching rules? The existing code in WI.SpreadsheetRulesStyleDetailsPanel attempts to save and restore the index of the non-matching rules (note that this code doesn’t actually work, see Comment 17 above). This would cause rather strange behavior when the matching rules change significantly.

I think modifying selector to something that doesn’t match the inspected element is relatively rare. Getting CSS classes added/removed after editing a selector to something that doesn’t match the inspected element must be very-very rare. I think we should go with the easy approach.

I have a working patch, and I’m working on the tests.

[Nikita Vasilyev]

Created attachment 407617 [details]
Patch

DOMNodeStyles.js changes here are the same as in the previous patch.
SpreadsheetRulesStyleDetailsPanel.js changes are new.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #20)
> EASY
> Show only matching rules. That’s what both Chrome DevTools and Firefox
> DevTools do.

The most straightforward way of implementing this would be to add a flag like _ignoreNextLayout in SpreadsheetRulesStyleDetailsPanel, that is added after editing a selector. This might have worked, but in my experience introducing a state like this tends to produce more error prone code. I didn't pursue this approach.

I've gone a slightly different route of implementing the easy approach (see the patch).

[Blaze Burg]

Comment on attachment 407617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review

> Source/WebInspectorUI/ChangeLog:15
> +        the same place as it was before r243264.

Nit: explain that if the map isn't cleared, then it is not possible to diff old and new style declarations.

> Source/WebInspectorUI/ChangeLog:21
> +        exiting exiting layout early.

Nit: exiting exiting

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345
> +    _getStyleIds(styles)

It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this

    styles.authorStyles().map((style) => style.stringId)

That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:348
> +            // FIXME: <https://webkit.org/b/110055> Web Inspector: User Agent style stylesheet identifier keeps incrementing

Not sure why this FIXME is relevant. Can you explain or remove it?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:378
> +        console.assert(found, "Could not find style rule of edited selector.");

Nit: please include the relevant object(s) as extra arguments to console.assert. Here it would be this._renderedStyleIds and section.style.stringId

[Nikita Vasilyev]

Comment on attachment 407617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review

>> Source/WebInspectorUI/ChangeLog:15
>> +        the same place as it was before r243264.
> 
> Nit: explain that if the map isn't cleared, then it is not possible to diff old and new style declarations.

Good point.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345
>> +    _getStyleIds(styles)
> 
> It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this
> 
>     styles.authorStyles().map((style) => style.stringId)
> 
> That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055?

Yes, it's a workaround for https://webkit.org/b/110055. I check if the style ids have changed on line 227. Unfortunately, the UserAgent style ids increment every time and are unreliable. It's safe to exclude them here because UserAgent styles don't get affected by CSS classes being added/removed.

[Devin Rousso]

Comment on attachment 407617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345
>>> +    _getStyleIds(styles)
>> 
>> It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this
>> 
>>     styles.authorStyles().map((style) => style.stringId)
>> 
>> That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055?
> 
> Yes, it's a workaround for https://webkit.org/b/110055. I check if the style ids have changed on line 227. Unfortunately, the UserAgent style ids increment every time and are unreliable. It's safe to exclude them here because UserAgent styles don't get affected by CSS classes being added/removed.

That isn't entirely accurate.  There is UserAgent CSS for media controls that adjusts depending on a class, not to mention rules that may/not apply if other things are changed (e.g. the tag of a node, the `type` of an `input`, etc.) although these may already be considered a `significantChange` for other reasons.

[Nikita Vasilyev]

Yeah, my patch has an issue with changing <input type=checkbox> to <input type=text> - styles don't update :(
I tried it on http://nv.github.io/webkit-inspector-bugs/all-element-types.html.

Just curious, what's UserAgent CSS for media controls that adjusts depending on a class?

[Nikita Vasilyev]

Changing <input type=checkbox> to <input type=text> doesn't update styles and it has been broken long before r243264. To fix that, we really need to fix Bug 110055 - Web Inspector: User Agent style stylesheet identifier keeps incrementing.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #26)
> Yeah, my patch has an issue with changing <input type=checkbox> to <input type=text> - styles don't update :(
> I tried it on http://nv.github.io/webkit-inspector-bugs/all-element-types.html.
> 
> Just curious, what's UserAgent CSS for media controls that adjusts depending on a class?

I dunno off the top of my head, but I wouldn't be surprised if you could find a handful of examples in Source/WebCore/modules/modern-media-controls/

[Nikita Vasilyev]

Created attachment 407694 [details]
Patch

cq- because there's still no tests.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #27)
> Changing <input type=checkbox> to <input type=text> doesn't update styles
> and it has been broken long before r243264. To fix that, we really need to
> fix Bug 110055 - Web Inspector: User Agent style stylesheet identifier keeps
> incrementing.

I filed "bug 216040: Web Inspector: Elements: styles don't update for certain cases related to UserAgent styles" for this particular case.

[Nikita Vasilyev]

Created attachment 407715 [details]
Patch

[Blaze Burg]

Comment on attachment 407715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review

r=me, great work!

> LayoutTests/ChangeLog:9
> +        Added a test to veryfy that WI.DOMNodeStyles.Event.Refreshed fires with appropriate

Nit: verify

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:47
> +        this._ignoreLayoutAfterSelectorChange = false;

Nit: please rename. We typically name this 'suppress' if it's stopping something from happening. i.e., this._suppressLayoutAfterSelectorChange.

[Nikita Vasilyev]

Created attachment 407718 [details]
Patch

[Devin Rousso]

Comment on attachment 407715 [details]
Patch

nice UI fix, well done :)

> LayoutTests/inspector/css/node-styles-refreshed.html:12
> +        test(resolve, reject) {

NIT: can we make these tests `async` and use `await` instead?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:45
> +        this._renderedStyleIds = null;

NIT: not used

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:224
> +        let uniqueOrderedStyles = this.nodeStyles.uniqueOrderedStyles;

NIT: I think you can undo this move now

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229
> +            if (this._selectedNodeId && this._selectedNodeId === this.nodeStyles.node.id)
> +                return;

Is this still needed?  I don't think a selector change will also change the `this.nodeStyles.node.id`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366
> +        this._ignoreLayoutAfterSelectorChange = true;

Rather than ignoring the next `layout`, can we use `cancelLayout` instead?

[Nikita Vasilyev]

Comment on attachment 407715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review

>> LayoutTests/inspector/css/node-styles-refreshed.html:12
>> +        test(resolve, reject) {
> 
> NIT: can we make these tests `async` and use `await` instead?

Where would I use `await` in this test? I'm a bit of a noob here.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:45
>> +        this._renderedStyleIds = null;
> 
> NIT: not used

Thanks!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:224
>> +        let uniqueOrderedStyles = this.nodeStyles.uniqueOrderedStyles;
> 
> NIT: I think you can undo this move now

Yep.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229
>> +                return;
> 
> Is this still needed?  I don't think a selector change will also change the `this.nodeStyles.node.id`.

Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366
>> +        this._ignoreLayoutAfterSelectorChange = true;
> 
> Rather than ignoring the next `layout`, can we use `cancelLayout` instead?

Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing.

[Devin Rousso]

Comment on attachment 407715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review

>>> LayoutTests/inspector/css/node-styles-refreshed.html:12
>>> +        test(resolve, reject) {
>> 
>> NIT: can we make these tests `async` and use `await` instead?
> 
> Where would I use `await` in this test? I'm a bit of a noob here.

Fundamentally, tests are very synchronous in the sense that the output has to match the expectation file exactly, meaning that there is an order to "all the things".  `async` and `await` allow you to write asynchronous code in such a way that it appears synchronous, even though it's not.

In the case of these tests, because `WI.Object` event listeners fire synchronously, we know that the `Promise` returned from `nodeStyles.refresh()` will resolve _after_ `WI.DOMNodeStyles.Event.Refreshed` is dispatched, meaning we know that the `InspectorTest.expectTrue` will be executed before the `finally`.  As such, we can simply change this test to be like so:
```
    async test() {
        nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
            InspectorTest.expectTrue(event.data.significantChange, `Adding '.baz' class should cause a significant change.`);
        });

        InspectorTest.evaluateInPage(`document.body.classList.add("baz")`);

        await nodeStyles.refresh();
    }
```
IMO the majority of asynchronous Web Inspector tests could be written with `async` and be _far_ more readable than if they used `resolve`/`reject`.  I'd suggest searching for `async test()` in `LayoutTests/inspector/` (the changes I recently made for r266074 are a good example) and looking at how it's used elsewhere too :)

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229
>>> +                return;
>> 
>> Is this still needed?  I don't think a selector change will also change the `this.nodeStyles.node.id`.
> 
> Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline.

Right, but when will this code actually be used in practice?  A `layout` should happen after the selector is changed, so any future `layout` (such as if the node is changed) shouldn't have `this._ignoreLayoutAfterSelectorChange` set anyways.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366
>>> +        this._ignoreLayoutAfterSelectorChange = true;
>> 
>> Rather than ignoring the next `layout`, can we use `cancelLayout` instead?
> 
> Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing.

It's just reversing what's done in `WI.View._scheduleLayoutForView`.  It's possible that `cancelLayout` might not work, but if it does work I think that's preferred to keeping another property.

[Nikita Vasilyev]

Comment on attachment 407715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review

>>>> LayoutTests/inspector/css/node-styles-refreshed.html:12
>>>> +        test(resolve, reject) {
>>> 
>>> NIT: can we make these tests `async` and use `await` instead?
>> 
>> Where would I use `await` in this test? I'm a bit of a noob here.
> 
> Fundamentally, tests are very synchronous in the sense that the output has to match the expectation file exactly, meaning that there is an order to "all the things".  `async` and `await` allow you to write asynchronous code in such a way that it appears synchronous, even though it's not.
> 
> In the case of these tests, because `WI.Object` event listeners fire synchronously, we know that the `Promise` returned from `nodeStyles.refresh()` will resolve _after_ `WI.DOMNodeStyles.Event.Refreshed` is dispatched, meaning we know that the `InspectorTest.expectTrue` will be executed before the `finally`.  As such, we can simply change this test to be like so:
> ```
>     async test() {
>         nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
>             InspectorTest.expectTrue(event.data.significantChange, `Adding '.baz' class should cause a significant change.`);
>         });
> 
>         InspectorTest.evaluateInPage(`document.body.classList.add("baz")`);
> 
>         await nodeStyles.refresh();
>     }
> ```
> IMO the majority of asynchronous Web Inspector tests could be written with `async` and be _far_ more readable than if they used `resolve`/`reject`.  I'd suggest searching for `async test()` in `LayoutTests/inspector/` (the changes I recently made for r266074 are a good example) and looking at how it's used elsewhere too :)

Neat! I like it.

>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229
>>>> +                return;
>>> 
>>> Is this still needed?  I don't think a selector change will also change the `this.nodeStyles.node.id`.
>> 
>> Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline.
> 
> Right, but when will this code actually be used in practice?  A `layout` should happen after the selector is changed, so any future `layout` (such as if the node is changed) shouldn't have `this._ignoreLayoutAfterSelectorChange` set anyways.

Oh, I see what you're saying. This extra precaution would make sense if there were cases when `layout` doesn't happen after editing a selector. I haven't found such cases so it should be safe to remove it.

[Nikita Vasilyev]

Created attachment 407741 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 407715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review

>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366
>>>> +        this._ignoreLayoutAfterSelectorChange = true;
>>> 
>>> Rather than ignoring the next `layout`, can we use `cancelLayout` instead?
>> 
>> Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing.
> 
> It's just reversing what's done in `WI.View._scheduleLayoutForView`.  It's possible that `cancelLayout` might not work, but if it does work I think that's preferred to keeping another property.

`cancelLayout` doesn't work here because it cancels a scheduled layout. Here, the layout hasn't been scheduled yet.

[EWS]

Committed r266451: <https://trac.webkit.org/changeset/266451>

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