Summary: Frontend should be made to expect and handle disabled properties See related: <https://webkit.org/b/166786> Web Inspector: Toggling CSS Properties in Styles Sidebar (comment / uncomment) The backend can inform the frontend about commented out CSS Properties within a rule as disabled properties. The frontend doesn't expect these yet. It should be made to handle them and we should have tests for expected behavior in different cases of comments within a rule: Examples of cases where a comment may contain a property and may send the frontend information about contents in the comment: 1. A single commented out property: div { /* color: blue; */ background: green; } 2. Multiple commented out properties: div { /* color: blue; background: green; */ } 3. Multi-line comments div { /* color: blue; */ background: green; } The frontend will need to handle these disabled properties in some cases. Most importantly CSSStyleDeclarationTextEditor generates checkboxes to toggle individual properties and it currently do not know how to handle disabled properties and makes an absolute mess of things! There may be other places in the frontend that simply iterate properties without considering if the property is enabled/disabled.
Created attachment 298259 [details] [WIP] Work in Progress This patch is a starter. I believe it gets the Rules sidebar working as expected, but it needs real tests and heavy usage testing and code auditing to see if anywhere else in the inspector we are iterating properties and may be mishandling disabled properties (!property.enabled).
<rdar://problem/34379593>
<rdar://problem/34379735>
Created attachment 320575 [details] Patch Run on bots.
Attachment 320575 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:11: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320585 [details] Patch
Comment on attachment 320585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320585&action=review > Source/WebCore/ChangeLog:11 > + Add new test cases to inspector/css/css-property.html. I think the normal syntax prepare-ChangeLog generates is: Tests: inspector/css/css-property.html inspector/css/matched-style-properties.html > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:222 > + get allProperties() { return this._allProperties; } I see that `visibleProperties` below filters on _properties and not _allProperties. Do disabled properties have a styleDeclarationTextRange? If so we might end up including a allVisibleProperties in the future. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:311 > - let styleEnabled = event && event.data && event.data.enabled; > + let styleEnabled = event && event.data && event.data.attached; This doesn't make sense. `enabled` is the event data associated with the VisualStyleSelectorTreeItem CheckboxChanged event: this.dispatchEventToListeners(WI.VisualStyleSelectorTreeItem.Event.CheckboxChanged, {enabled: this._checkboxElement.checked}); This should not change to `attached`.
Comment on attachment 320585 [details] Patch Thank you for the review! View in context: https://bugs.webkit.org/attachment.cgi?id=320585&action=review >> Source/WebCore/ChangeLog:11 >> + Add new test cases to inspector/css/css-property.html. > > I think the normal syntax prepare-ChangeLog generates is: > > Tests: inspector/css/css-property.html > inspector/css/matched-style-properties.html Hm, it certainly didn't generate this for me. I'll add it manually. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:222 >> + get allProperties() { return this._allProperties; } > > I see that `visibleProperties` below filters on _properties and not _allProperties. Do disabled properties have a styleDeclarationTextRange? If so we might end up including a allVisibleProperties in the future. Good point. In the current styles sidebar, disabled (commented out) properties aren't included in _properties at all. In the new Styles sidebar, I want to deprecate visibleProperties. My understanding is they are *editable* properties, and whether a property is editable or not should be decided at a rule/declaration level. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:311 >> + let styleEnabled = event && event.data && event.data.attached; > > This doesn't make sense. `enabled` is the event data associated with the VisualStyleSelectorTreeItem CheckboxChanged event: > > this.dispatchEventToListeners(WI.VisualStyleSelectorTreeItem.Event.CheckboxChanged, {enabled: this._checkboxElement.checked}); > > This should not change to `attached`. My bad 🤦♂️
Created attachment 320680 [details] Patch
Comment on attachment 320680 [details] Patch Clearing flags on attachment: 320680 Committed r221993: <http://trac.webkit.org/changeset/221993>
All reviewed patches have been landed. Closing bug.