New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 612363 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Shorthand serialization should do generic checks (e.g. for initial) in a generic way

Project Member Reported by timloh@chromium.org, May 17 2016

Issue description

Currently shorthand serialization has different functions for serializing which all need to handle checks like checking all values are present, checking the !important flag is set identically across longhands, checking for the 'initial' value, and so on. Ideally these would all be done in a single place for all shorthands.

While this is primarily motivated by code health, this is also likely to fix many inconsistencies where the generic checks are implemented slightly wrongly in different places.

This will also make it easier to implement var() references in shorthands correctly.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e

commit 5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e
Author: timloh <timloh@chromium.org>
Date: Wed May 18 13:58:00 2016

Fix 'border' serialization to fail in cssText when it is invalid

Currently we serialize 'border' correctly in e.style.border, but when
reading cssText (which serializes an entire declaration) we sometimes
get this wrong. Since the 'border' shorthand always sets the four
(top bottom left right) longhands for each type (color style width)
to the same value, if any of the types have unequal longhands then
seialization should fail.

The goal here is to make cssText serialization do the same thing as
the other serialization, so we can make it just call getPropertyValue
and have generic checks (e.g. for 'initial') in that function.

This patch makes us match Firefox in behaviour.

BUG= 612363 

Review-Url: https://codereview.chromium.org/1982903002
Cr-Commit-Position: refs/heads/master@{#394409}

[modify] https://crrev.com/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e/third_party/WebKit/LayoutTests/fast/css/getPropertyValue-border-expected.txt
[modify] https://crrev.com/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e/third_party/WebKit/LayoutTests/fast/css/getPropertyValue-border.html
[modify] https://crrev.com/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e/third_party/WebKit/LayoutTests/fast/dom/css-shorthand-common-value.html
[modify] https://crrev.com/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e/third_party/WebKit/Source/core/css/StylePropertySerializer.cpp
[modify] https://crrev.com/5cf5e5234ad01ef8e22f10f25f674232a2ae7d2e/third_party/WebKit/Source/core/css/StylePropertySerializer.h

Project Member

Comment 2 by bugdroid1@chromium.org, May 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df

commit ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df
Author: timloh <timloh@chromium.org>
Date: Thu May 19 03:32:35 2016

Fix up CSSStyleDeclaration::cssText serialization

This patch fixes up CSSStyleDeclaration::cssText serialization in
several ways:
- Removes switch statement and just defers to getPropertyValue. This will
    allow us to move generic checks (e.g. for initial) up so individual
    shorthand serialization routines don't need to handle it.
- Supports all shorthands (except font) now. This adds support for
    marker, columns, column-rule, font-variant, -webkit-margin-collapse,
    -webkit-mask-repeat, -webkit-text-stroke.
- Support serialization as border-top/bottom/left/right. Currently we
    only use border and border-width/color/style. As per CSSOM, we
    prefer (e.g.) border-width to border-top as it has more longhands.
- Avoid serializing border-image when border is serialized as
    border-image is a reset-only sub-property (but border serialization
    still fails to require border-image to be correctly set).

https://www.w3.org/TR/cssom-1/#concept-shorthands-preferred-order

BUG= 612363 

Review-Url: https://codereview.chromium.org/1984163002
Cr-Commit-Position: refs/heads/master@{#394653}

[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/fast/css/all-shorthand-css-text-expected.txt
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/fast/css/all-shorthand-css-text.html
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/fast/css/cssText-shorthand.html
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/fast/css/remove-shorthand-expected.txt
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/fast/css/webkit-mask-crash-implicit-expected.txt
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/LayoutTests/inspector/console/console-format-style-whitelist-expected.txt
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/Source/build/scripts/make_style_shorthands.py
[modify] https://crrev.com/ee9bb4bbcd3eecff24fc8d11b21147e8a3b0c5df/third_party/WebKit/Source/core/css/StylePropertySerializer.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f45f2b736d341572758154cc9c37614ac4a013c

commit 9f45f2b736d341572758154cc9c37614ac4a013c
Author: timloh <timloh@chromium.org>
Date: Mon May 23 08:54:06 2016

Move generic shorthand serialization checks out of specific routines

This patch moves generic shorthand serialization checks out of the
specific shorthand serialization routines into a single location.
For a shorthand to successfully serialize, all longhands must be set
and the important flag must be set consistently.

If all longhands are the same css-wide keyword (initial or inherit,
unset not yet supported properly), then the shorthand serializes to
that keyword. If css-wide keywords are otherwise used, the serialization
of a shorthand should fail, but currently we allow initial for certain
properties.

This patch also makes a couple of other changes. These changes would
make some behavior incorrect if they are left out or done separately.
- No longer skip serializing implicit initial values in cssText. This
    affects the case where a shorthand is set and other longhands are
    overridden so we can't serialize as a shorthand.
- Remove separate background serialization codepath. Currently we call
    appendBackgroundPropertyAsText() when serializing cssText if any
    background properties are present, which serializes either the
    background shorthand or the longhands individually. This is removed
    in this patch so we just treat background like any other shorthand,
    so background properties are no longer moved to the end of cssText
    and now ordered as output by the parser.

This patch will make it easier to fix serialization of unset, and also
fix variables references in shorthands ( bug 612634 ).

BUG= 612363 ,  471917 

Review-Url: https://codereview.chromium.org/1988013003
Cr-Commit-Position: refs/heads/master@{#395295}

[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/deleting/merge-div-from-span-with-style-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/deleting/merge-paragraph-from-span-with-style-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/pasteboard/dragstart-contains-default-content-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/pasteboard/onpaste-text-html-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/style/push-down-inline-styles-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/editing/style/script-tests/push-down-inline-styles.js
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/background-serialize-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/background-serialize.html
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/cssText-shorthand.html
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/parse-border-image-repeat-null-crash-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/remove-shorthand-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/css/webkit-mask-crash-implicit-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/dom/Range/range-clone-contents-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/LayoutTests/fast/events/ondrop-text-html-expected.txt
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/Source/core/css/StylePropertySerializer.cpp
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/Source/core/css/StylePropertySerializer.h
[modify] https://crrev.com/9f45f2b736d341572758154cc9c37614ac4a013c/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Comment 4 by timloh@chromium.org, May 24 2016

Status: Fixed (was: Started)

Sign in to add a comment