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.
<rdar://problem/48658929>
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.
Regressed in https://trac.webkit.org/changeset/240946/webkit.
Created attachment 364224 [details] Patch Not for landing. I need to add tests first.
Created attachment 364465 [details] Patch
Created attachment 364467 [details] [Video] With patch applied
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.
Created attachment 364567 [details] Patch for landing
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.
Created attachment 364573 [details] Patch for landing
Comment on attachment 364573 [details] Patch for landing Clearing flags on attachment: 364573 Committed r242914: <https://trac.webkit.org/changeset/242914>
All reviewed patches have been landed. Closing bug.
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