Bug 166786 – REGRESSION(r208886) Web Inspector: Toggling CSS Properties in Styles Sidebar (comment / uncomment)

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

[Joseph Pecoraro]

Summary:
Toggling CSS Properties in Styles Sidebar (comment / uncomment)

Modifications enabled in by the new CSS Parser backend (r208886 and turned on in r209396) introduced some issues toggling CSS properties in Web Inspector's Style Sidebar.

Steps to Reproduce: Toggling an individual CSS Properties
1. Inspect the div.post.odd of <http://bogojoker.com/shell/>
2. Show the Styles > Rules in the Sidebar
3. Check / Uncheck the checkbox for the "background: #EFF2FF;" property for the ".odd" rule
  => Does not behave as expected

Steps to Reproduce: Toggling a group of Properties
1. Inspect the div.post.odd of <http://bogojoker.com/shell/>
2. Show the Styles > Rules in the Sidebar
3. Click the [{}] icon for some of the rules
  => Does not behave as expected

Notes:
- The root cause here is the backend is sending new, potentially useful, data to the frontend that the frontend doesn't know how to deal with. The frontend ends up dealing with it very poorly.

As a first step, this bug will handle getting us back to the previous behavior.

We can then follow-up with a bug that will fix the frontend to handle the new data.

[Joseph Pecoraro]

<rdar://problem/29767412>

[Joseph Pecoraro]

*** Bug 166297 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 165831 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

For some context. Toggling commenting out a property the frontend makes the same modification, but the response changed.

    [Log] request: {
        "id": 157,
        "method": "CSS.setStyleText",
        "params": {
            "styleId": {
                "styleSheetId": "4",
                "ordinal": 4
            },
            "text": "\n    /* background: #EFF2FF; */\n"
        }
    }

Before the new path the frontend just got the text with no information about commented "disabled" properties:

    [Log] response: {
        "result": {
            "style": {
                "cssProperties": [],
                "shorthandEntries": [],
                "styleId": { "styleSheetId": "16", "ordinal": 4 },
                "range": {
                    "startLine": 9,
                    "startColumn": 6,
                    "endLine": 11,
                    "endColumn": 0
                },
                "cssText": "\n    /* background: #EFF2FF; */\n"
            }
        },
        "id": 361
    }

Now in trunk we get extra information about the commented out property!

    [Log] response: {
        "result": {
            "style": {
                "cssProperties": [{
                    "name": "background",
                    "value": "#EFF2FF",
                    "text": "/* background: #EFF2FF; */",
                    "status": "disabled"
                }],
                "shorthandEntries": [],
                "styleId": { "styleSheetId": "4", "ordinal": 4 },
                "range": {
                    "startLine": 9,
                    "startColumn": 6,
                    "endLine": 11,
                    "endColumn": 0
                },
                "cssText": "\n    /* background: #EFF2FF; */\n"
            }
        },
        "id": 157
    }

The frontend isn't properly handling these kinds of (status "disabled") properties.

Ultimately we should, this is added data that may be useful in the future. But fixing the frontend is a much larger and riskier change.

As a quick fix, to keep the new CSS parser enabled, lets get the backend back to what it was doing and investigate taking advantage of the new data in the frontend.

[Joseph Pecoraro]

Created attachment 298253 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 298253 [details]
[PATCH] Proposed Fix

r=me

Is this exercised by a test? Please consider to write one for the fix of 166787.

[Joseph Pecoraro]

(In reply to comment #6)
> Comment on attachment 298253 [details]
> [PATCH] Proposed Fix
> 
> r=me
> 
> Is this exercised by a test?

No because this is disabling the new behavior that will be enabled once we address the FIXME. Meaning, we don't currently support and haven't had to support status "disabled" previously, and this change brings us back in line with that.

> Please consider to write one for the fix of 166787.

Yep that is the plan. When we enable this on the backend and frontend we should have tests for this since the protocol messages will be different (containing information they didn't contain before).

[WebKit Commit Bot]

Comment on attachment 298253 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 298253

Committed r210472: <http://trac.webkit.org/changeset/210472>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.