Bug 211467 - Web Inspector: Styles: User Agent shorthand properties displayed incorrectly
Summary: Web Inspector: Styles: User Agent shorthand properties displayed incorrectly
Status: ASSIGNED
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: 212919
  Show dependency treegraph
 
Reported: 2020-05-05 13:16 PDT by Nikita Vasilyev
Modified: 2022-03-25 12:33 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 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.
Comment 1 Radar WebKit Bug Importer 2020-05-05 13:16:53 PDT
<rdar://problem/62900473>
Comment 2 Nikita Vasilyev 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.
Comment 3 Nikita Vasilyev 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.
Comment 4 Nikita Vasilyev 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
Comment 5 Nikita Vasilyev 2020-05-26 03:31:05 PDT
Created attachment 400235 [details]
[Image] With patch applied
Comment 6 Nikita Vasilyev 2020-05-28 07:27:58 PDT
Created attachment 400453 [details]
Patch
Comment 7 Devin Rousso 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.
Comment 8 Nikita Vasilyev 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.
Comment 9 Devin Rousso 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"}
        ]
    }
```