Bug 157180 - Specifying a longhand property should not serialize to a shorthand property
Summary: Specifying a longhand property should not serialize to a shorthand property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-29 06:43 PDT by Antoine Quint
Modified: 2016-05-02 19:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (27.40 KB, patch)
2016-04-29 07:46 PDT, Antoine Quint
dino: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-04-29 06:43:39 PDT
The CSS spec defines what should happen when serializing a CSS declaration block at http://www.w3.org/TR/cssom-1/#serialize-a-css-declaration-block. In there, the spec says that we should only serialize to a shorthand property if all required longhand properties are specified. So an element with a "style" attribute set to "background-color: green" should not return "green" when querying element.style.background, but "".
Comment 1 Radar WebKit Bug Importer 2016-04-29 06:44:04 PDT
<rdar://problem/26002754>
Comment 2 Antoine Quint 2016-04-29 07:46:42 PDT
Created attachment 277700 [details]
Patch
Comment 3 Dean Jackson 2016-04-29 17:48:28 PDT
Comment on attachment 277700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277700&action=review

> Source/WebCore/css/StyleProperties.cpp:389
> +        // We don't have all longhand properties defined as required for the shorthand
> +        // property and thus should not serialize to a shorthand value. See spec at
> +        // http://www.w3.org/TR/cssom-1/#serialize-a-css-declaration-block.
> +        if (!values[i])
> +            return String();

Put this comment inside the if statement, so it doesn't read as applying to everything here.

> LayoutTests/fast/css/no-shorthand-with-incomplete-longhands.html:9
> +var shorthands = { "background": ["background-image: url(foo.png)", "background-position-x: 10px", "background-position-y: 10px", "background-size: cover", "background-repeat-x: no-repeat", "background-repeat-y: no-repeat", "background-attachment: scroll", "background-origin: padding-box", "background-clip: padding-box", "background-color: green"] }

Shouldn't you try a few more shorthands? Like animation, transition, border, margin, etc
Comment 4 Simon Fraser (smfr) 2016-05-02 15:34:54 PDT
Does this regress bug 81737?
Comment 5 Dean Jackson 2016-05-02 15:47:55 PDT
This would cause pasted content to get more verbose inline style.

I think we need a way for editing workflows to keep this behaviour, but allow our Web-facing APIs to be compliant.
Comment 6 Ryosuke Niwa 2016-05-02 18:42:42 PDT
(In reply to comment #5)
> This would cause pasted content to get more verbose inline style.
> 
> I think we need a way for editing workflows to keep this behaviour, but
> allow our Web-facing APIs to be compliant.

I thought we had a different code path for editing?  Specifically, I don't see this patch changing http://trac.webkit.org/browser/trunk/LayoutTests/editing/pasteboard/paste-and-sanitize.html.
Comment 7 Dean Jackson 2016-05-02 18:58:42 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > This would cause pasted content to get more verbose inline style.
> > 
> > I think we need a way for editing workflows to keep this behaviour, but
> > allow our Web-facing APIs to be compliant.
> 
> I thought we had a different code path for editing?  Specifically, I don't
> see this patch changing
> http://trac.webkit.org/browser/trunk/LayoutTests/editing/pasteboard/paste-
> and-sanitize.html.

Great, then I'll go ahead and land it.
Comment 8 Dean Jackson 2016-05-02 19:25:14 PDT
Committed r200357: <http://trac.webkit.org/changeset/200357>