Bug 211467

Summary:

Web Inspector: Styles: User Agent shorthand properties displayed incorrectly

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

ASSIGNED

Severity:

Normal

CC:

hi, inspector-bugzilla-changes, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

212919

Attachments:

Description	Flags
Patch	none
[Image] With patch applied	none
Patch	hi: review-, nvasilyev: commit-queue-

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	Details Formatted Diff Diff
[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-	Details Formatted Diff Diff
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.