ASSIGNED 211467
Web Inspector: Styles: User Agent shorthand properties displayed incorrectly 
https://bugs.webkit.org/show_bug.cgi?id=211467
Summary Web Inspector: Styles: User Agent shorthand properties displayed incorrectly
Nikita Vasilyev
Reported 2020-05-05 13:16:41 PDT
Steps: 1. Inspect <body> 2. In the style editor, you should see... Expected: body { display: block; margin: 8px; } or this body { display: block; margin-top: 8px; margin-right: 8px; margin-bottom: 8px; margin-left: 8px; } Actual: body { display: block; margin-top: 8px; } Notes: I'm guessing this isn't just margin but all User Agent shortcut properties. We seem to display only the 1st longhand property of the shorthand.
Attachments
Patch (4.74 KB, patch)
2020-05-26 03:25 PDT, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
[Image] With patch applied (30.14 KB, image/png)
2020-05-26 03:31 PDT, Nikita Vasilyev 		no flags
Details
Patch (7.83 KB, patch)
2020-05-28 07:27 PDT, Nikita Vasilyev 		hi: review-
nvasilyev: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-05-05 13:16:53 PDT
<rdar://problem/62900473>
Nikita Vasilyev
Comment 2 2020-05-07 01:17:42 PDT
Not a recent regression. I vaguely remember this working correctly; it must've been over 12 months ago.
Nikita Vasilyev
Comment 3 2020-05-07 01:18:00 PDT
Back-end response: ``` { cssProperties: [ {name: "display", value: "block"}, {name: "margin-top", value: "8px"}, {name: "margin-right", value: "8px", implicit: true}, {name: "margin-bottom", value: "8px", implicit: true}, {name: "margin-left", value: "8px", implicit: true} ], shorthandEntries: [ {name: "margin", value: "8px"} ] } ``` "margin-top" does NOT include "implicit: true". -right, -bottom, and -left do for some reason. Regardless, the shorthand is available and we should show it.
Nikita Vasilyev
Comment 4 2020-05-26 03:25:16 PDT
Created attachment 400234 [details] Patch I still need to write tests. Here's a page for manual testing: http://nv.github.io/webkit-inspector-bugs/all-element-types.html
Nikita Vasilyev
Comment 5 2020-05-26 03:31:05 PDT
Created attachment 400235 [details] [Image] With patch applied
Nikita Vasilyev
Comment 6 2020-05-28 07:27:58 PDT
Created attachment 400453 [details] Patch
Devin Rousso
Comment 7 2020-06-03 08:05:45 PDT
Comment on attachment 400453 [details] Patch r-, this seems like the wrong approach. We should figure out why `implicit` is being set in the backend for the other longhand properties and fix that.
Nikita Vasilyev
Comment 8 2020-06-08 14:05:08 PDT
(In reply to Devin Rousso from comment #7) > Comment on attachment 400453 [details] > Patch > > r-, this seems like the wrong approach. We should figure out why `implicit` > is being set in the backend for the other longhand properties and fix that. I'd like to display longhands as children of shorthands. I should've explicitly mention it here. Bug 212919: Web Inspector: Styles: display longhand properties under shorthands If we agree that it's a good idea, this patch would be a stepping a stepping stone as it takes (currently unused) shorthandEntries data from the backend payload. `implicit` property would be irrelevant.
Devin Rousso
Comment 9 2020-06-11 17:45:51 PDT
(In reply to Nikita Vasilyev from comment #8) > (In reply to Devin Rousso from comment #7) > > Comment on attachment 400453 [details] > > Patch > > > > r-, this seems like the wrong approach. We should figure out why `implicit` is being set in the backend for the other longhand properties and fix that. > > I'd like to display longhands as children of shorthands. I should've explicitly mention it here. > > Bug 212919: Web Inspector: Styles: display longhand properties under shorthands That sounds like a good feature, although I'm not sure what form it'll take given how dense the Styles panel already is, so we'd probably want to brainstorm/iterate that. > If we agree that it's a good idea, this patch would be a stepping a stepping stone as it takes (currently unused) shorthandEntries data from the backend payload. `implicit` property would be irrelevant. You're more than welcome to use anything from this patch as a stepping stone. The logic as written in this patch, however, essentially covers up a bigger issue, which is that UserAgent styles are being incorrectly marked as implicit (i.e. this bug). We should figure out why this is happening and fix it. Furthermore, the style as written in `html.css` is actually `margin: 8px;`, so in reality _all_ of the longhand properties should be implicit and the shorthand property should _not_ be implicit. Either there's something different about how UserAgent styles are parsed/applied or there is a bug in the inspector backend that causes this incorrect behavior. I would expect to see a payload similar to: ``` { cssProperties: [ {name: "display", value: "block"}, {name: "margin", value: "8px"}, {name: "margin-top", value: "8px", implicit: true}, {name: "margin-right", value: "8px", implicit: true}, {name: "margin-bottom", value: "8px", implicit: true}, {name: "margin-left", value: "8px", implicit: true} ], shorthandEntries: [ {name: "margin", value: "8px"} ] } ```
Note You need to log in before you can comment on or make changes to this bug.