New issue
Advanced search Search tips

Issue 816722 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 545318



Sign in to add a comment

[css-typed-om] Handle shorthands correctly.

Project Member Reported by shend@chromium.org, Feb 27 2018

Issue description

The spec currently doesn't say much about shorthands. We'll be supporting it as a base CSSStyleValue object.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 28 2018

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

commit 41f6273211e9c08770594af70f25594ec242c6eb
Author: Darren Shen <shend@chromium.org>
Date: Wed Feb 28 00:11:22 2018

[css-typed-om] Implement shorthands for .get/.getAll/.has.

This patch implements shorthands for stylemap.get/getAll/has. Shorthands
are implemented as a base CSSStyleValue. They are different to other
properties because they cannot be converted to a single CSSValue.
Instead, we need to store the CSSValues of their longhands so that
they can be set again on the same property.

Patches to come:
- styleMap.set/update
- Serialization of these values.

Bug: 816722
Change-Id: Ia2c3f694809c219566b965ebfae40bc38d74f4c5
Reviewed-on: https://chromium-review.googlesource.com/938881
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539594}
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/computed/get-shorthand.html
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/computed/getAll-shorthand.html
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/computed/has.tentative.html
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/get-shorthand.html
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/getAll-shorthand.html
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/has.tentative.html
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/get-shorthand.html
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/getAll-shorthand.html
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/has.tentative.html
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h
[add] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/Source/core/css/cssom/CSSUnsupportedShorthandValue.h
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/Source/core/css/cssom/StylePropertyMapReadOnly.cpp
[modify] https://crrev.com/41f6273211e9c08770594af70f25594ec242c6eb/third_party/WebKit/Source/core/css/cssom/StylePropertyMapReadOnly.h

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 1 2018

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

commit 73513536d1ea775571307d2b112ac3668125151d
Author: Darren Shen <shend@chromium.org>
Date: Thu Mar 01 02:19:16 2018

[css-typed-om] Implement shorthands for StylePropertyMap.set/delete.

This patch implements shorthands for stylemap.set/delete. When we
receive an UnsupportedShorthandProperty, we should get the CSSValues
out of them and set them on the style map.

We leave parsing of shorthand values for a future patch.

Bug: 816722
Change-Id: Iebd0b33f89b1d8872bea5d3f80d1dde71d72bc45
Reviewed-on: https://chromium-review.googlesource.com/938684
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539992}
[modify] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/append.tentative.html
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/delete-shorthand.html
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/set-shorthand-expected.txt
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/set-shorthand.html
[modify] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/append.tentative.html
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/delete-shorthand.html
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/set-shorthand-expected.txt
[add] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/set-shorthand.html
[modify] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/margin.html
[modify] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp
[modify] https://crrev.com/73513536d1ea775571307d2b112ac3668125151d/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.h

Comment 3 by shend@chromium.org, Mar 1 2018

Cc: shend@chromium.org
 Issue 812914  has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 1 2018

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

commit 69a0ee5fd58190a6d2ba5e21898d4886053c521a
Author: Darren Shen <shend@chromium.org>
Date: Thu Mar 01 04:04:50 2018

[css-typed-om] Implement serialization for CSSUnsupportedStyleValues.

This patch implements serialization for CSSUnsupportedStyleValue.
The spec requires that:

- CSSStyleValues that are parsed from string should serialize to the
  given string.
- CSSStyleValues that are obtained from CSSOM should serialize according
  to [1] (although this is mostly the same as CSSOM serialization).

To implement both, we put a String on CSSStyleValue (the base class)
to store its serialization. This is only used by the subclass
CSSUnsupportedStyleValue. When we create a CSSUnsupportedStyleValue,
we set the serialization correctly depending on if it's from a string
or CSSOM. CSSUnsupportedStyleValue.toString() just outputs the stored
string.

It turns out that the string on CSSStyleValue can completely replace
its CSSValue. Instead of wrapping a CSSValue, which is fragile when
passed around arbitrarily, we simply parse the stored string whenever
we need to convert to a CSSValue. This actually was the original
design, but we changed it because we thought styleMap.set cannot
invoke parsing. Since we resolved to not expose styleMap.set to
worklets, we are allowed to parse and this is a much more robust
solution.

Serialization for shorthands requires different logic depending on
if it's a computed shorthand or a specified shorthand. This meant
that we had to create a new method for getting the serialization
and override it for different property maps.

We don't need CSSUnsupportedShorthandValue anymore, since we can
can just use CSSUnsupportedStyleValue.

[1] https://drafts.css-houdini.org/css-typed-om-1/#cssom-serialization

Bug: 816722
Change-Id: Ia236f72467e414a2eb380f00ddb196db06f8d8b6
Reviewed-on: https://chromium-review.googlesource.com/940283
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540039}
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-objects/parse-invalid.html
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-objects/parse.html
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-objects/parseAll-invalid.html
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-objects/parseAll.html
[add] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-serialization/cssStyleValue-cssom.html
[add] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-serialization/cssStyleValue-string.html
[delete] https://crrev.com/eea28440cd07c597fef380f9c6dbfb9278502f30/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/set-shorthand-expected.txt
[delete] https://crrev.com/eea28440cd07c597fef380f9c6dbfb9278502f30/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/set-shorthand-expected.txt
[add] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/background.html
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/CSSStyleValue.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h
[delete] https://crrev.com/eea28440cd07c597fef380f9c6dbfb9278502f30/third_party/WebKit/Source/core/css/cssom/CSSUnsupportedShorthandValue.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/CSSUnsupportedStyleValue.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/CSSUnsupportedStyleValue.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/DeclaredStylePropertyMap.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/DeclaredStylePropertyMap.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/PrepopulatedComputedStylePropertyMap.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/PrepopulatedComputedStylePropertyMap.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/StylePropertyMapReadOnly.cpp
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/StylePropertyMapReadOnly.h
[modify] https://crrev.com/69a0ee5fd58190a6d2ba5e21898d4886053c521a/third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 1 2018

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

commit 48dc8954a17282693a5a57e6e1768c289aaadf19
Author: Darren Shen <shend@chromium.org>
Date: Thu Mar 01 04:54:23 2018

[css-typed-om] Ensure properties works with css-wide keywords / var refs

There are two things that should always be supported with any property:
- css-wide keywords like 'initial'
- var refs like 'var(--A)'

This patch adds tests to ensure that this works for all the different
types of properties. Unfortunately, supporting this in shorthands
is quite difficult (we get shorthands as strings, so to reify them
we have to parse, but we can't parse in reification) so we'll leave
that for the future.

Bug: 816722
Change-Id: I1d344b73d011db81cfc3880be54823302534fd36
Reviewed-on: https://chromium-review.googlesource.com/942121
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540055}
[add] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-normalization/normalize-tokens.tentative-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-normalization/normalize-tokens.tentative.html
[add] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/background-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/background.html
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/font-style-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/font-weight-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/line-height-expected.txt
[add] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/margin-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/margin.html
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/mask-image-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/outline-style-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/overflow-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/resources/testsuite.js
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/shape-outside-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/text-emphasis-color-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/text-transform-expected.txt
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/Source/build/scripts/core/css/templates/CSSOMTypes.cpp.tmpl
[modify] https://crrev.com/48dc8954a17282693a5a57e6e1768c289aaadf19/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp

Comment 6 by shend@chromium.org, Mar 1 2018

Only problem left is that css-wide keywords / var refs are reified as base CSSStyleValues. We can upgrade them to their correct class when this is implemented.

Comment 7 by shend@chromium.org, May 3 2018

Owner: ----
Status: Available (was: Started)
No longer working on Typed OM, marking as Available.

Sign in to add a comment