Bug 139750
| Summary: | Web Inspector: show user agent style properties as shorthand if it looks better than longhand | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> |
| Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | bburg, graouts, inspector-bugzilla-changes, jonowells, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
Joseph Pecoraro
* 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 */
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/19285473>
Joseph Pecoraro
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
}
Joseph Pecoraro
This is especially ugly for something like <textarea>!
Timothy Hatcher
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.
Timothy Hatcher
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.
Joseph Pecoraro
(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.
Joseph Pecoraro
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
Joseph Pecoraro
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.