Example: border-color: red; border: 1px solid green; The 1st property should be displayed as overridden and have a strikethrough. <rdar://problem/45341525>
<rdar://problem/51504160>
Created attachment 371536 [details] Patch Tip for reviewers: LayoutTests/inspector/css/overridden-property.html doubles as a reduction page. You can open it in an old Safari to see how it worked before, and in ToT WebKit with the patch applied to see what changed.
Attachment 371536 [details] did not pass style-queue: ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312: Line contains single-quote character. [js/syntax] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #3) > Attachment 371536 [details] did not pass style-queue: > > > ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312: Line > contains single-quote character. [js/syntax] [5] > Total errors found: 1 in 6 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. `Property "${this.formattedText}" can't override itself.` ^ It didn't like this single-quote :/
Comment on attachment 371536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371536&action=review As a side note, I'd expect a shorthand property to be overridden if ALL of its shorthands are present and override this property, but that's probably going to be more work, so it may deserve its own followup. > LayoutTests/inspector/css/overridden-property-expected.txt:13 > +border-top-color: red; overridden These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected. > LayoutTests/inspector/css/overridden-property.html:10 > + if (property.implicit) Should this instead be an `InspectorTest.assert`? I'd expect that `matchedRule.style.enabledProperties` shouldn't contain any implicit properties. > LayoutTests/inspector/css/overridden-property.html:47 > + if (rule.selectorText === selector) { Why not use `WI.DOMNodeStyles.prototype.rulesForSelector`? Although, it doesn't appear to be used anywhere, so you could also just remove it :P > LayoutTests/inspector/css/overridden-property.html:54 > + InspectorTest.log(`Couldn't find ${selector}`); This should be an `InspectorTest.fail`. > LayoutTests/inspector/css/overridden-property.html:113 > + name: "NotOverriddenByLonghandImportant", We also have a `NotOverriddeenByLonghand` for more test coverage/completeness. > LayoutTests/inspector/css/overridden-property.html:-71 > - <p>Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.</p> There should always be a description in the test that explains overall what the test is for. > LayoutTests/inspector/css/overridden-property.html:134 > + .shorthand-important { These names aren't very clear. If anything i'd expect this to be `.shorthand-overridden-by-important-longhand`. > LayoutTests/inspector/css/overridden-property.html:139 > + .longhand-important { Ditto (>134), but with `.longhand-overridden-by-important-shorthand` instead. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:302 > + console.assert(this !== effectiveProperty, `Property "${this.formattedText}" can't override itself.`); NIT: I'd also log `this` as another argument so that if inspector2 is already open, we can do more investigation immediately. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:862 > + else { Style: no `else` when the `if` is an early-return. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:907 > + if (isOverriddenBy(property, property.relatedShorthandProperty)) { To prevent future foot-guns, I think we should rename this function to be `isOverriddenByRelatedShorthand` and have the logic for fetching `property.relatedShorthandProperty` be inside the function itself. The reason for this is that we don't want to mark a shorthand property as overridden if only one of it's longhands exists (see >overridden-property.html:140).
(In reply to Nikita Vasilyev from comment #4) > (In reply to Build Bot from comment #3) > > Attachment 371536 [details] did not pass style-queue: > > > > > > ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312: Line contains single-quote character. [js/syntax] [5] > > Total errors found: 1 in 6 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > `Property "${this.formattedText}" can't override itself.` > ^ > It didn't like this single-quote :/ This is definitely a bug :( Please file a bug against check-webkit-style.
(In reply to Devin Rousso from comment #6) > (In reply to Nikita Vasilyev from comment #4) > > (In reply to Build Bot from comment #3) > > > Attachment 371536 [details] did not pass style-queue: > > > > > > > > > ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312: Line contains single-quote character. [js/syntax] [5] > > > Total errors found: 1 in 6 files > > > > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > > > `Property "${this.formattedText}" can't override itself.` > > ^ > > It didn't like this single-quote :/ > This is definitely a bug :( > > Please file a bug against check-webkit-style. I filed a related style checker bug a while back: <https://webkit.org/b/165453> check-webkit-style: false positive caused by regex literal matching in SingleQuoteChecker
Comment on attachment 371536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371536&action=review Thanks for reviewing! >> LayoutTests/inspector/css/overridden-property-expected.txt:13 >> +border-top-color: red; overridden > > These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected. Where would you put a "PASS"? It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing. >> LayoutTests/inspector/css/overridden-property.html:10 >> + if (property.implicit) > > Should this instead be an `InspectorTest.assert`? I'd expect that `matchedRule.style.enabledProperties` shouldn't contain any implicit properties. No, matchedRule.style.enabledProperties does contain implicit properties. `visibleProperties` does NOT. I could use either, I don't have a preference.
Comment on attachment 371536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371536&action=review >>> LayoutTests/inspector/css/overridden-property-expected.txt:13 >>> +border-top-color: red; overridden >> >> These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected. > > Where would you put a "PASS"? > > It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing. I mean using the `InspectorText.expect*` functions. It would show as a PASS if they match, and a FAIL if not. I had to read the expected file a few times before I understood what it was trying to tell me. The point of the PASS is to make it clear to _other_ people what they should be looking for. Sometimes, dumping the raw text makes sense. I don't think that's the case here because it's not obvious what the "overridden" means since the rest of the text is real CSS.
(In reply to Devin Rousso from comment #9) > Comment on attachment 371536 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371536&action=review > > >>> LayoutTests/inspector/css/overridden-property-expected.txt:13 > >>> +border-top-color: red; overridden > >> > >> These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected. > > > > Where would you put a "PASS"? > > > > It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing. > > I mean using the `InspectorText.expect*` functions. It would show as a PASS > if they match, and a FAIL if not. I had to read the expected file a few > times before I understood what it was trying to tell me. Some tests are less straightforward to express in the form "actual vs. expected". Many of our tests log stack traces or other state, and just compare it against future logs. In general these are more difficult to read and understand, and rely on the author ensuring the initial output is correct. Tests of the form "actual vs. expected" are a bit better in both regards, IMO. > The point of the PASS is to make it clear to _other_ people what they should > be looking for. Sometimes, dumping the raw text makes sense. I don't think > that's the case here because it's not obvious what the "overridden" means > since the rest of the text is real CSS.
Comment on attachment 371536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371536&action=review >> LayoutTests/inspector/css/overridden-property.html:54 >> + InspectorTest.log(`Couldn't find ${selector}`); > > This should be an `InspectorTest.fail`. Whatever you do, you should resolve/reject the promise or call InspectorTest.completeTest, so that the test doesn't time out.
Created attachment 371613 [details] Patch
Attachment 371613 [details] did not pass style-queue: ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312: Line contains single-quote character. [js/syntax] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 371613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371613&action=review r=me, with some final fixes before landing. > LayoutTests/inspector/css/overridden-property-expected.txt:15 > +PASS: border-top-color is overridden. Nice! This is MUCH easier to read/understand. > LayoutTests/inspector/css/overridden-property.html:38 > + function getStyleDeclaration(selector, callback, resolve) { Now that you're passing a `callback`, I'd replace the `resolve` with a `reject`, as we really should be throwing an error if we couldn't find the selector. > LayoutTests/inspector/css/overridden-property.html:92 > + name: "OverriddenByShorthand", Style: please prefix the test case name with the suite name, so "OverriddenProperty.OverriddenByShorthand". > LayoutTests/inspector/css/overridden-property.html:95 > + InspectorTest.expectTrue(style.visibleProperties[0].overridden, "border-top-color is overridden."); We should be using `propertyForName` instead of expecting that it's the first visible property. ``` const dontCreateIfMissing = true; let borderTopColorProperty = style.propertyForName("border-top-color", dontCreateIfMissing); ``` Alternatively, adding an `InspectorTest.assert(style.visibleProperties[0].name === "border-top-color");` would be fine too. > LayoutTests/inspector/css/overridden-property.html:120 > + }); Shouldn't you also pass `reject` in here? > LayoutTests/inspector/css/overridden-property.html:131 > + }); Ditto (>120). > LayoutTests/inspector/css/overridden-property.html:146 > + .shorthand { A better name could be used here, like `.longhand-overridden-by-shorthand`. > LayoutTests/inspector/css/overridden-property.html:161 > + .shorthand-not-overridden-longhand { NIT: this is missing a `-by-`, so `.shorthand-not-overridden-by-longhand`. > LayoutTests/inspector/css/overridden-property.html:170 > + <div class="shorthand-overridden-by-important-longhand"></div> > + <div class="shorthand-overridden-by-important-longhand"></div> Duplicate. Please remove (or explain why it's still needed).
Created attachment 371618 [details] Patch
Comment on attachment 371618 [details] Patch Clearing flags on attachment: 371618 Committed r246223: <https://trac.webkit.org/changeset/246223>
All reviewed patches have been landed. Closing bug.