Bug 166787 - Web Inspector: Frontend should be made to expect and handle disabled properties
Summary: Web Inspector: Frontend should be made to expect and handle disabled properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks: 176643
  Show dependency treegraph
 
Reported: 2017-01-06 17:00 PST by Joseph Pecoraro
Modified: 2017-09-13 13:48 PDT (History)
8 users (show)

See Also:


Attachments
[WIP] Work in Progress (15.26 KB, patch)
2017-01-06 22:33 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (10.47 KB, patch)
2017-09-12 15:47 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (15.87 KB, patch)
2017-09-12 17:27 PDT, Nikita Vasilyev
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Patch (15.01 KB, patch)
2017-09-13 13:01 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-01-06 17:00:38 PST
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.
Comment 1 Joseph Pecoraro 2017-01-06 22:33:46 PST
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).
Comment 2 Nikita Vasilyev 2017-09-11 16:26:00 PDT
<rdar://problem/34379593>
Comment 3 Radar WebKit Bug Importer 2017-09-11 16:31:43 PDT
<rdar://problem/34379735>
Comment 4 Nikita Vasilyev 2017-09-12 15:47:14 PDT
Created attachment 320575 [details]
Patch

Run on bots.
Comment 5 Build Bot 2017-09-12 15:59:36 PDT
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.
Comment 6 Nikita Vasilyev 2017-09-12 17:27:04 PDT
Created attachment 320585 [details]
Patch
Comment 7 Joseph Pecoraro 2017-09-13 11:46:31 PDT
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 8 Nikita Vasilyev 2017-09-13 12:48:20 PDT
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 🤦‍♂️
Comment 9 Nikita Vasilyev 2017-09-13 13:01:21 PDT
Created attachment 320680 [details]
Patch
Comment 10 WebKit Commit Bot 2017-09-13 13:48:12 PDT
Comment on attachment 320680 [details]
Patch

Clearing flags on attachment: 320680

Committed r221993: <http://trac.webkit.org/changeset/221993>
Comment 11 WebKit Commit Bot 2017-09-13 13:48:14 PDT
All reviewed patches have been landed.  Closing bug.