Bug 195389 - REGRESSION(r240946): Web Inspector: Styles: removing selected property doesn't update overridden status
Summary: REGRESSION(r240946): Web Inspector: Styles: removing selected property doesn'...
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-06 16:58 PST by Nikita Vasilyev
Modified: 2019-03-13 15:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.50 KB, patch)
2019-03-10 19:59 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (5.46 KB, patch)
2019-03-12 16:11 PDT, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
[Video] With patch applied (152.01 KB, video/quicktime)
2019-03-12 16:17 PDT, Nikita Vasilyev
no flags Details
Patch for landing (5.53 KB, patch)
2019-03-13 13:25 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch for landing (5.53 KB, patch)
2019-03-13 14:30 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-03-06 16:58:12 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-03-06 16:59:40 PST
<rdar://problem/48658929>
Comment 2 Nikita Vasilyev 2019-03-06 23:08:58 PST
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.
Comment 3 Nikita Vasilyev 2019-03-10 19:53:40 PDT
Regressed in https://trac.webkit.org/changeset/240946/webkit.
Comment 4 Nikita Vasilyev 2019-03-10 19:59:01 PDT
Created attachment 364224 [details]
Patch

Not for landing. I need to add tests first.
Comment 5 Nikita Vasilyev 2019-03-12 16:11:28 PDT
Created attachment 364465 [details]
Patch
Comment 6 Nikita Vasilyev 2019-03-12 16:17:31 PDT
Created attachment 364467 [details]
[Video] With patch applied
Comment 7 Matt Baker 2019-03-13 13:10:52 PDT
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.
Comment 8 Nikita Vasilyev 2019-03-13 13:25:28 PDT
Created attachment 364567 [details]
Patch for landing
Comment 9 Matt Baker 2019-03-13 13:55:40 PDT
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.
Comment 10 Nikita Vasilyev 2019-03-13 14:30:56 PDT
Created attachment 364573 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2019-03-13 15:09:34 PDT
Comment on attachment 364573 [details]
Patch for landing

Clearing flags on attachment: 364573

Committed r242914: <https://trac.webkit.org/changeset/242914>
Comment 12 WebKit Commit Bot 2019-03-13 15:09:37 PDT
All reviewed patches have been landed.  Closing bug.