Bug 139750 - Web Inspector: show user agent style properties as shorthand if it looks better than longhand
Summary: Web Inspector: show user agent style properties as shorthand if it looks bett...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-12-17 15:04 PST by Joseph Pecoraro
Modified: 2017-10-13 14:40 PDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-12-17 15:04:33 PST
* SUMMARY
<body> shows unexpected UserAgent margin styles (one non-implicit, others implicit). I would expect them all to be the same, the reason User Agent stylesheet says "margin: 8px".

* STEPS TO REPRODUCE
1. Inspect <body> in data:text/html,<body>
2. Show Rules in Styles details sidebar
  => "margin" values shown in unexpected way

* ACTUAL RESULTS:
display: block;     /* non-implicit */
margin-top: 8px;    /* non-implicit */
margin-right: 8px;  /* implicit */
margin-bottom: 8px; /* implicit */
margin-left: 8px;   /* implicit */

* EXPECTED RESULTS:
display: block;     /* non-implicit */
margin: 8px;        /* non-implicit */
Comment 1 Radar WebKit Bug Importer 2014-12-17 15:04:49 PST
<rdar://problem/19285473>
Comment 2 Joseph Pecoraro 2014-12-17 15:19:46 PST
Looks like an issue with the backend reply for CSS.getMatchedStylesForNode dealing with shorthanded entries. For "margin: 8px" it sends some of the long-hands as implicit.

frontend: {
    "method": "CSS.getMatchedStylesForNode",
    "params": {
        "nodeId": 8,
        "includePseudo": true,
        "includeInherited": true
    },
    "id": 52
}

backend: {
    "result": {
        "matchedCSSRules": [{
            "rule": {
                "selectorList": {
                    "selectors": [{
                        "text": "body",
                        "specificity": [0, 0, 1]
                    }],
                    "text": "body"
                },
                "sourceLine": 0,
                "origin": "user-agent",
                "style": {
                    "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"
                    }],
                    "styleId": {
                        "styleSheetId": "4",
                        "ordinal": 2
                    },
                    "width": "",
                    "height": ""
                }
            },
            "matchingSelectors": [0]
        }],
        ...
    },
    "id": 52
}
Comment 3 Joseph Pecoraro 2015-01-06 15:55:59 PST
This is especially ugly for something like <textarea>!
Comment 4 Timothy Hatcher 2015-01-06 16:04:33 PST
I agree we should just show margin: 8px here. We don't need to show the long-hands.

The backend reporting "implicit" is correct here. The only value specified specifically is margin-top. The rest are implied from that one value.

For example: margin: 8px 4px; would yield margin-top and margin-right with 8px and 4px, and the margin-bottom and margin-left would be implicit.

What use to do was show the shorthand, with the long-hands expandable under it. I'm not sure we should do that again, and just show the shorthand by itself.
Comment 5 Timothy Hatcher 2015-01-06 16:11:33 PST
We don't have the original source for the user agent rules, which causes us to synthesize the source.

We just need to be smarter and the long-hands that are covered by the shorthandEntries list when synthesizing the source.
Comment 6 Joseph Pecoraro 2015-01-06 16:32:21 PST
(In reply to comment #4)
> The backend reporting "implicit" is correct here. The only value specified
> specifically is margin-top. The rest are implied from that one value.

I don't think that is true. The User-Agent styles are:

    /* Source/WebCore/css/html.css */
    body {
        display: block;
        margin: 8px
    }

margin-top is no more specified then margin-left or the others. In my opinion they should either all be implicit or not.

I guess I don't understand why WebKit says otherwise:

    var list = document.defaultView.getMatchedCSSRules(document.body);
    var rule = list[2];
    var decl = rule.style;
    rule.cssText; // "body { font-family: Verdana, Arial, Helvetica; font-size: 12px; padding: 0px 5px; margin: 0px; }"
    decl.isPropertyImplicit("margin"); // false
    decl.isPropertyImplicit("margin-top") // false
    decl.isPropertyImplicit("margin-left") // true
    decl.isPropertyImplicit("margin-right") // true
    decl.isPropertyImplicit("margin-bottom") // true

I guess I don't actually know why something is considered implicit, or there is a bug.


> For example: margin: 8px 4px; would yield margin-top and margin-right with
> 8px and 4px, and the margin-bottom and margin-left would be implicit.
> 
> What use to do was show the shorthand, with the long-hands expandable under
> it. I'm not sure we should do that again, and just show the shorthand by
> itself.

I agree with just showing the shorthand.

--

Here is an interesting case. Is there ever a case with ambiguity like this?

    div {
        margin: 5px;
        margin-right: 10px;
    }

If there is a case like this in non-author styles, I think we should be unambiguous, and probably list everything.
Comment 7 Joseph Pecoraro 2015-01-06 16:49:31 PST
After chatting with Timothy Hatcher on IRC.

Implicit properties are ones not present in the shorthand. e.g. in "margin: 5px" only "top" is present and the rest are implicit, whereas in "margin: 5px 5px 5px 5px" they are all present.

In that case, there are a few changes here.

- I don't think showing transparent "implicit" values is useful data in the "Rules" section of the Styles sidebar. We should drop that.

- We should show the best user visible string to the user
  - if a longhand value is overridden by author styles, show all longhand values to show exactly what is overridden
  - otherwise, it is safe to show the shorthand, but perhaps only show shorthands for simple properties like "margin/padding", not "font/background" which are more complicated for developers to remember exactly what it set

See related:
<https://webkit.org/b/140154> Web Inspector: Expected UserAgent styles to be crossed-out if overridden
Comment 8 Joseph Pecoraro 2015-01-06 18:12:35 PST
Addressing transparency with:
<https://webkit.org/b/140161> Web Inspector: Do not style implicit CSS properties in the Style Rules section

Leaving this open to handle the actual logic for shorthand vs longhand.

Also, it appears we are over-loading the term "implicit", which makes this even more confusing, but that is also a separate issue.