It's often hard to find why the property is strikethrough, and what overrode it. To solve this, display an arrow up icon after the overridden CSS property. Clicking this icon would scroll up to either: 1. The active property (highest priority property). 2. Next property with the higher priority, but not necessary the active property. If this is hard to implement, [1] is fine. After scrolling to the property, focus on the property and highlight it (the same way we highlight the properties when jumping from the Computed panel). This has been requested several times. AFAIK, neither Chrome DevTools nor Firefox DevTools have this.
<rdar://problem/40506252>
Created attachment 363569 [details] Patch
Created attachment 363570 [details] [Video] With patch applied
(In reply to Nikita Vasilyev from comment #3) > Created attachment 363570 [details] > [Video] With patch applied Nice! To reduce UI clutter, should we only reveal the arrow on mouseover?
Comment on attachment 363569 [details] Patch Attachment 363569 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11371958 New failing tests: fast/scrolling/ios/hit-testing-iframe-004.html fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html fast/scrolling/ios/hit-testing-iframe-001.html
Created attachment 363598 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
(In reply to Matt Baker from comment #4) > (In reply to Nikita Vasilyev from comment #3) > > Created attachment 363570 [details] > > [Video] With patch applied > > Nice! To reduce UI clutter, should we only reveal the arrow on mouseover? Yeah, I do like that!
Created attachment 363610 [details] Patch
Comment on attachment 363610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363610&action=review r- because of a couple general comments: I'm worried that CSSProperty _overridden and _overriddenBy could get out of sync. What about dropping _overridden and replacing the public property with: get overridden() { return !!this._overriddingProperty; } You'd need to eliminate the WI.CSSProperty constructor argument `overridden`, or replaced it with `overriddingProperty`. This could be an issue in DOMNodeStyles.prototype._parseStylePropertyPayload, which constructs a WI.CSSProperty and sets overridden equal to true if payload.status.inactive. If this is too annoying to refactor, you can disregard this but be sure to fix the issue in CSSProperty.prototype.update. My other comment is style. 1) The spacing between the property value, goto arrow, and overriding property value feels too cramped. 2. The overriding property value hint should be shown with the arrow. It feels odd having it appear only when the goto arrow is hovered. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:34 > + this._overriddenBy = null; What about _overridingProperty? > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:266 > + this._overriddenBy = null; This should also be nulled out in update() when !overridden.
I wonder if we want a way to reveal/flash a property, like we do with source code locations. I was a little surprised that the property became selected after clicking the goto arrow. It's fine for now, but we might want to consider this for a follow-up.
Created attachment 363692 [details] Patch
(In reply to Matt Baker from comment #10) > I wonder if we want a way to reveal/flash a property, like we do with source > code locations. I was a little surprised that the property became selected > after clicking the goto arrow. It's fine for now, but we might want to > consider this for a follow-up. Yes, it would be a nice follow-up! We used to do that. It had several bugs. For instance, lines being stuck highlighted when the animation was interrupted (by switching to a different tab or panel, for instance). Regardless, I think we should bring it back and fix the bugs.
(In reply to Nikita Vasilyev from comment #11) > Created attachment 363692 [details] > Patch This doesn't address the hover or horizontal spacing issues.
(In reply to Matt Baker from comment #9) > Comment on attachment 363610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363610&action=review > > r- because of a couple general comments: > > I'm worried that CSSProperty _overridden and _overriddenBy could get out of > sync. What about dropping _overridden and replacing the public property with: > > get overridden() > { > return !!this._overriddingProperty; > } > > You'd need to eliminate the WI.CSSProperty constructor argument > `overridden`, or replaced it with `overriddingProperty`. This could be an > issue in DOMNodeStyles.prototype._parseStylePropertyPayload, which > constructs a WI.CSSProperty and sets overridden equal to true if > payload.status.inactive. If this is too annoying to refactor, you can > disregard this but be sure to fix the issue in CSSProperty.prototype.update. The current patch adds an experimental setting and tries to make as little as possible changes to the existing behavior. I agree with what you're saying! I think this refactoring should be done when the feature is turned on by default. > > My other comment is style. > > 1) The spacing between the property value, goto arrow, and overriding > property value feels too cramped. More than 40% of our users use Web Inspector docked to right with the styles sidebar being very narrow. What are you suggesting, exactly? > 2. The overriding property value hint should be shown with the arrow. It > feels odd having it appear only when the goto arrow is hovered. I tried it out and it didn't seem too obnoxious, so I think I agree with you.
Created attachment 363712 [details] Patch Addressed the hover. Left a comment about the horizontal spacing above.
Comment on attachment 363712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363712&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:300 > + if (!WI.settings.experimentalEnableStylesJumpToEffective.value) > + return; When this value changes, should we reset `_overridingProperty` to `null` (or force a reload of Web Inspector)? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921 > property.overridden = true; > + property.overridingProperty = effectiveProperty; Rather than have both an `overridden` and an `overridingProperty`, why don't we say that `get overridden` returns true if `overridingProperty` is non-null? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:926 > property.overridden = true; > + property.overridingProperty = effectiveProperty; Ditto (>920). > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:932 > effectiveProperty.overridden = true; > + effectiveProperty.overridingProperty = property; Ditto (>920). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:169 > +.spreadsheet-style-declaration-editor .property .select-effective-property { Does this sector match anything? I would think that we only create `.select-effective-property` when we're `.overridden`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:173 > + margin-left: 0; NIT: I try to keep this in between "sizing" (e.g. `width` and `height`) and "content" (e.g. `line-height`, `vertical-align`, etc.) properties. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:184 > + content: attr(data-value); NIT: I normally put this with other "content"-esque properties, like above `white-space`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:187 > + display: inline-block; NIT: I normally put this as the first property, since it can have the most impact/effect of other properties (e.g. `display: none;` trumps all other styles). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:224 > + this._jumpToEffectiveButton = arrowButton; Why not just use `this._jumpToEffectiveButton` instead of making another variable `arrowButton`? ``` this._jumpToEffectiveButton = WI.createGoToArrowButton(); this._jumpToEffectiveButton.classList.add("select-effective-property"); ... ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:279 > + this._jumpToEffectiveButton.dataset.value = this._property.overridingProperty ? this._property.overridingProperty.rawValue : ""; Shouldn't we always have an `overridingProperty`? Maybe just assert instead?
Comment on attachment 363712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363712&action=review >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921 >> + property.overridingProperty = effectiveProperty; > > Rather than have both an `overridden` and an `overridingProperty`, why don't we say that `get overridden` returns true if `overridingProperty` is non-null? Yes, I agree that would make things easier. However, currently, the overridden logic is split between the backend and the front-end. See _parseStylePropertyPayload and _markOverriddenProperties. If we move all the logic to the front-end, then I could make `get overridden` returns true if `overridingProperty` is non-null. If I do this, I'd want to write tests. I outlined some other concerns related to the existing `overridden` logic here: https://bugs.webkit.org/show_bug.cgi?id=195389#c2
Comment on attachment 363712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363712&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:300 >> + return; > > When this value changes, should we reset `_overridingProperty` to `null` (or force a reload of Web Inspector)? Good point. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:184 >> + content: attr(data-value); > > NIT: I normally put this with other "content"-esque properties, like above `white-space`. I still think `content` is the most important property. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:224 >> + this._jumpToEffectiveButton = arrowButton; > > Why not just use `this._jumpToEffectiveButton` instead of making another variable `arrowButton`? > ``` > this._jumpToEffectiveButton = WI.createGoToArrowButton(); > this._jumpToEffectiveButton.classList.add("select-effective-property"); > ... > ``` Ok, since this is what we usually do. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:279 >> + this._jumpToEffectiveButton.dataset.value = this._property.overridingProperty ? this._property.overridingProperty.rawValue : ""; > > Shouldn't we always have an `overridingProperty`? Maybe just assert instead? Yes, it should. Good point.
Created attachment 363854 [details] Patch
(In reply to Nikita Vasilyev from comment #19) > Created attachment 363854 [details] > Patch A few issues we observed offline, which need to be resolved: 1) The tip text with the overriding property value is hard to see in dark mode. This is compounded when the property has a warning, and has the warning color background color. 2) Goto arrow doesn't appear for overridden User Agent properties.
Created attachment 363938 [details] Patch
Comment on attachment 363938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363938&action=review r=me, with some suggestions. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:915 > +localizedStrings["Show Jump to Effective Property Buttons"] = "Show Jump to Effective Property Buttons"; This should be singular: "Show Jump to Effective Property Button". > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:299 > + if (!WI.settings.experimentalEnableStylesJumpToEffective.value) Should we be performing this check? I think we should always track the effective property, but only show the UI when the setting is enabled. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921 > + property.overridingProperty = effectiveProperty; You could eliminate some of the syncing of these two properties by doing this: class CSSProperty { set overridingProperty(property = null) { if (this._overridingProperty === property) return; this._overridingProperty = property; this._overridden = !!this._overridingProperty } } Unless it's possible that `overridden === true && overridingProperty === null`. Just a thought. If it doesn't simplify things, please disregard. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:226 > + /* .select-effective-property has inverted colors. Invert the text color again to make it readable. */ Suggestion: ".select-effective-property has inverted colors. Invert the pseudo-element again to restore the original text color." > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:42 > + this._jumpToEffectiveButton = null; this._jumpToEffectivePropertyButton > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:264 > if (this._property.overridden) { If it is not possible that `overridden && !overridingProperty`, I'd test this._overridingProperty instead. Then you can eliminate the assert and test below.
Created attachment 363943 [details] Patch for landing
Comment on attachment 363943 [details] Patch for landing Clearing flags on attachment: 363943 Committed r242622: <https://trac.webkit.org/changeset/242622>
All reviewed patches have been landed. Closing bug.