Bug 195389 – REGRESSION(r240946): Web Inspector: Styles: removing selected property doesn't update overridden status

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Add "color: red" property.
2. Add "color: blue" after it. "color: red" should have a strike-through now.
3. Select "color: blue" property.
4. Press Delete.

Actual:
"color: red" has a strike-through.

Expected:
"color: red" must no longer have a strike-through.

[Radar WebKit Bug Importer]

<rdar://problem/48658929>

[Nikita Vasilyev]

I've been working on the styles editor for over 2 years now, and I find `overridden` logic to be the most unnecessarily convoluted piece.

Some unsettling points:

1. `overridden` is first set from the backend payload (_parseStylePropertyPayload) and then calculated on the front-end (_markOverriddenProperties). I don't yet understand why it has to be this way.

2. There's a potential race condition in WI.DOMNodeStyles.prototype.refresh:

        CSSAgent.getMatchedStylesForNode.invoke({nodeId: this._node.id, includePseudo: true, includeInherited: true}, wrap.call(this, fetchedMatchedStyles, fetchedMatchedStylesPromise));
        CSSAgent.getInlineStylesForNode.invoke({nodeId: this._node.id}, wrap.call(this, fetchedInlineStyles, fetchedInlineStylesPromise));
        CSSAgent.getComputedStyleForNode.invoke({nodeId: this._node.id}, wrap.call(this, fetchedComputedStyle, fetchedComputedStylesPromise));

        this._pendingRefreshTask = Promise.all([fetchedMatchedStylesPromise.promise, fetchedInlineStylesPromise.promise, fetchedComputedStylesPromise.promise])
        .then(() => {
            this._pendingRefreshTask = null;
            return this;
        });

`overridden` properties get updated in fetchedInlineStyles, and not when all three promises finish. I don't yet understand if it was intentional.

3. I don't understand why CSSStyleDeclaration needed something like this:

        function delayed()
        {
            this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged);
        }

        // Delay firing the PropertiesChanged event so DOMNodeStyles has a chance to mark overridden and associated properties.
        setTimeout(delayed.bind(this), 0);

4. No tests.

[Nikita Vasilyev]

Regressed in https://trac.webkit.org/changeset/240946/webkit.

[Nikita Vasilyev]

Created attachment 364224 [details]
Patch

Not for landing. I need to add tests first.

[Nikita Vasilyev]

Created attachment 364465 [details]
Patch

[Nikita Vasilyev]

Created attachment 364467 [details]
[Video] With patch applied

[Matt Baker]

Comment on attachment 364465 [details]
Patch

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

r=me. The fix looks good, although I think we should add additional tests, such as:

1) Test that overriding a property causes it to be flagged as overridden
2) Test that overriding a property multiple times yields the correct effective property

And probably many more, but this is a good start. It looks like the FIXME will need to be addressed first. That would be a good time to expand this test suite.

> LayoutTests/inspector/css/overridden-property.html:37
> +                // OverriddenStatusChanged may fire more than once. FIXME: https://webkit.org/b/195651

Nit: // FIXME: <https://webkit.org/b/195651> OverriddenStatusChanged may fire more than once.

> LayoutTests/inspector/css/overridden-property.html:41
> +                InspectorTest.log("OverriddenStatusChanged");

Style: we usually log event listener entry with `InspectorTest.log("OverriddenStatusChanged event fired.")`.

Once the FIXME is resolved, I think we'll want to add:
InspectorTest.expectFalse(styleRuleProperty.overridden, "Style rule property should not be overridden.");

> LayoutTests/inspector/css/overridden-property.html:50
> +            if (contentNodeId) {

I'd use an early return here:

if (!contentNodeId) {
    InspectorTest.fail("DOM node not found.");
    InspectorTest.completeTest();
    return;
}

> LayoutTests/inspector/css/overridden-property.html:70
> +    <p>Testing that CSSProperty.prototype.overridden updates as expected.</p>

I suggest re-wording this: "Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed."

Note: Use the imperative form of the verb instead of present continuous:  "Testing" -> "Test". It's a bit more common (94 uses vs 75 uses) and IMO, sounds more correct.

> LayoutTests/inspector/css/overridden-property.html:71
> +

Unnecessary blank line.

[Nikita Vasilyev]

Created attachment 364567 [details]
Patch for landing

[Matt Baker]

Comment on attachment 364567 [details]
Patch for landing

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

> LayoutTests/inspector/css/overridden-property.html:61
> +                    suite.runTestCasesAndFinish()

Missing semicolon.

[Nikita Vasilyev]

Created attachment 364573 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364573 [details]
Patch for landing

Clearing flags on attachment: 364573

Committed r242914: <https://trac.webkit.org/changeset/242914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

New test inspector/css/overridden-property.html is a flaky failure on the bots.

Is a flaky failure on Mac WK2 Debug and Mojave Release.

I cannot reproduce locally. I have tried 5000 iterations in release and debug and no failure. Also tried running with the test list from the worker for failed runs and still cannot reproduce.

Diff from Bots: 

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/css/overridden-property-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/css/overridden-property-actual.txt
@@ -1,5 +1,9 @@
 Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.
 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
+ERROR: Received a command response without a corresponding callback or promise. [object Object] 
 
 == Running test suite: OverriddenProperty
 -- Running test case: OverriddenProperty.effectivePropertyRemoved