166786 2017-01-06 16:50:52 -0800 REGRESSION(r208886) Web Inspector: Toggling CSS Properties in Styles Sidebar (comment / uncomment) 2017-01-06 22:54:58 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck commit-queue dino hyatt inspector-bugzilla-changes joepeck nvasilyev webkit-bug-importer oldest_to_newest 1264573 0 joepeck 2017-01-06 16:50:52 -0800 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. 1264574 1 joepeck 2017-01-06 16:51:09 -0800 <rdar://problem/29767412> 1264577 2 joepeck 2017-01-06 17:04:08 -0800 *** Bug 166297 has been marked as a duplicate of this bug. *** 1264579 3 joepeck 2017-01-06 17:04:53 -0800 *** Bug 165831 has been marked as a duplicate of this bug. *** 1264583 4 joepeck 2017-01-06 17:12:19 -0800 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. 1264634 5 298253 joepeck 2017-01-06 20:12:47 -0800 Created attachment 298253 [PATCH] Proposed Fix 1264640 6 298253 bburg 2017-01-06 20:27:11 -0800 Comment on attachment 298253 [PATCH] Proposed Fix r=me Is this exercised by a test? Please consider to write one for the fix of 166787. 1264657 7 joepeck 2017-01-06 22:29:05 -0800 (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). 1264659 8 298253 commit-queue 2017-01-06 22:54:53 -0800 Comment on attachment 298253 [PATCH] Proposed Fix Clearing flags on attachment: 298253 Committed r210472: <http://trac.webkit.org/changeset/210472> 1264660 9 commit-queue 2017-01-06 22:54:58 -0800 All reviewed patches have been landed. Closing bug. 298253 2017-01-06 20:12:47 -0800 2017-01-06 22:54:53 -0800 [PATCH] Proposed Fix comments-1.patch text/plain 1907 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxM2U5MzdhLi42YzliYTU4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTctMDEtMDYgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwor ICAgICAgICBSRUdSRVNTSU9OKHIyMDg4ODYpIFdlYiBJbnNwZWN0b3I6IFRvZ2dsaW5nIENTUyBQ cm9wZXJ0aWVzIGluIFN0eWxlcyBTaWRlYmFyIChjb21tZW50IC8gdW5jb21tZW50KQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY2Nzg2CisgICAgICAg IDxyZGFyOi8vcHJvYmxlbS8yOTc2NzQxMj4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIGluc3BlY3Rvci9JbnNwZWN0b3JTdHlsZVNoZWV0LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6Okluc3BlY3RvclN0eWxlOjpwb3B1bGF0ZUFsbFByb3BlcnRpZXMp OgorICAgICAgICBNYXRjaCBlYXJsaWVyIGJlaGF2aW9yIGFuZCBkb24ndCBzZW5kIHRoZSBmcm9u dGVuZCBpbmZvcm1hdGlvbiBhYm91dAorICAgICAgICBkaXNhYmxlZCAoY29tbWVudGVkIG91dCkg cHJvcGVydGllcy4gRm9sbG93LXVwIGxhdGVyIHRvIHNlbmQgdGhpcworICAgICAgICBpbmZvcm1h dGlvbiB3aGVuIGltcGxlbWVudGluZyBwcm9wZXIgaGFuZGxpbmcgb2YgdGhlbSBpbiB0aGUgZnJv bnRlbmQuCisKIDIwMTctMDEtMDYgIFphbGFuIEJ1anRhcyAgPHphbGFuQGFwcGxlLmNvbT4KIAog ICAgICAgICBUZXh0IGhpZ2hsaWdodCBjYXVzZXMgWW9vbiBHb3RoaWMgd2ViZm9udCB0byByZWZs b3cuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvSW5zcGVjdG9yU3R5bGVT aGVldC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvSW5zcGVjdG9yU3R5bGVTaGVldC5j cHAKaW5kZXggYzkyMTdmYi4uZTNkYTliMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaW5z cGVjdG9yL0luc3BlY3RvclN0eWxlU2hlZXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2luc3Bl Y3Rvci9JbnNwZWN0b3JTdHlsZVNoZWV0LmNwcApAQCAtNjA0LDYgKzYwNCw5IEBAIHZvaWQgSW5z cGVjdG9yU3R5bGU6OnBvcHVsYXRlQWxsUHJvcGVydGllcyhWZWN0b3I8SW5zcGVjdG9yU3R5bGVQ cm9wZXJ0eT4qIHJlc3VsCiAgICAgICAgIEFTU0VSVCghc3R5bGVEZWNsYXJhdGlvbk9yRXhjZXB0 aW9uLmhhc0V4Y2VwdGlvbigpKTsKICAgICAgICAgU3RyaW5nIHN0eWxlRGVjbGFyYXRpb24gPSBz dHlsZURlY2xhcmF0aW9uT3JFeGNlcHRpb24uaGFzRXhjZXB0aW9uKCkgPyBlbXB0eVN0cmluZygp IDogc3R5bGVEZWNsYXJhdGlvbk9yRXhjZXB0aW9uLnJlbGVhc2VSZXR1cm5WYWx1ZSgpOwogICAg ICAgICBmb3IgKGF1dG8mIHNvdXJjZURhdGEgOiAqc291cmNlUHJvcGVydHlEYXRhKSB7CisgICAg ICAgICAgICAvLyBGSVhNRTogPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzE2Njc4Nz4gV2ViIEluc3Bl Y3RvcjogRnJvbnRlbmQgc2hvdWxkIGJlIG1hZGUgdG8gZXhwZWN0IGFuZCBoYW5kbGUgZGlzYWJs ZWQgcHJvcGVydGllcworICAgICAgICAgICAgaWYgKHNvdXJjZURhdGEuZGlzYWJsZWQpCisgICAg ICAgICAgICAgICAgY29udGludWU7CiAgICAgICAgICAgICBJbnNwZWN0b3JTdHlsZVByb3BlcnR5 IHAoc291cmNlRGF0YSwgdHJ1ZSwgc291cmNlRGF0YS5kaXNhYmxlZCk7CiAgICAgICAgICAgICBw LnNldFJhd1RleHRGcm9tU3R5bGVEZWNsYXJhdGlvbihzdHlsZURlY2xhcmF0aW9uKTsKICAgICAg ICAgICAgIHJlc3VsdC0+YXBwZW5kKHApOwo=