Bug 166787 – Web Inspector: Frontend should be made to expect and handle disabled properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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).

[Nikita Vasilyev]

<rdar://problem/34379593>

[Radar WebKit Bug Importer]

<rdar://problem/34379735>

[Nikita Vasilyev]

Created attachment 320575 [details]
Patch

Run on bots.

[Build Bot]

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.

[Nikita Vasilyev]

Created attachment 320585 [details]
Patch

[Joseph Pecoraro]

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`.

[Nikita Vasilyev]

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 🤦‍♂️

[Nikita Vasilyev]

Created attachment 320680 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320680 [details]
Patch

Clearing flags on attachment: 320680

Committed r221993: <http://trac.webkit.org/changeset/221993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.