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

Issue 816402 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocking:
issue 545318



Sign in to add a comment

[css-typed-om] Whitelist supported properties

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

Issue description

The current Typed OM code will do its best to support any CSS property. This works most of the time, but can sometimes get the computed value wrong (as computed style code may return the used value instead).

We should be more disciplined and explicitly whitelist properties that we support.
 

Comment 1 by shend@chromium.org, Feb 26 2018

Cc: zhuoyu.q...@samsung.com nikhil.s...@samsung.com hs1217....@samsung.com
Hi folks, can anyone take this? It should be pretty simple to check whether a property is supported or not in StyleValueFactory::CreateStyleValueWithProperty with like a switch statement or something. Only properties with tests should be supported [1].

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/?q=the-stylepropertymap/properties&sq=package:chromium&dr
if we were add check logic for whitelist, 
when unsupported property was passed in StyleValueFactory::CreateStyleValueWithProperty, what we do?
do we should just return CreateStyleValue(nullptr) like present implement,
or do we have to throw NotSupportError?

is whitelist should have same list with the list in spec[1] eventually?
[1]https://drafts.css-houdini.org/css-typed-om-1/#reify-property-style-value

Comment 3 by shend@chromium.org, Feb 26 2018

We should return an CSSUnsupportedStyleValue, so that it can be upgraded to a full fledged CSSStyleValue subclass when we do support it.

Eventually yeah, the whitelist should be the same as the spec. It'll be a large effort to achieve parity with the spec, so we should just properly ship what we can and no more.
Status: Assigned (was: Available)
i will take this issue.
Owner: hs1217....@samsung.com
Status: Started (was: Assigned)
@shend
i am working do process to add whitelist. and i have a question.
some properties like "transition-duration" was missed in "the-stylepropertymap/properties/"
but "transition-duration" was used to make test in wpt("stylevalue-objects/parseAll.html")
also it is included in spec's list
(https://drafts.css-houdini.org/css-typed-om-1/#reify-property-style-value) 
so that properties should be included in whitelist?
and also do we have to add test for missed properties in "the-stylepropertymap/properties/"

Comment 8 by shend@chromium.org, Feb 28 2018

Ah yes, sorry, the-properties/properties/ is probably incomplete. Feel free to add a test for those (there's probably < 5 missing?). Good catch!

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

Hi Hwanseung, I was looking to this, and it seems like this is more difficult than I thought because a few properties that we didn't plan to ship are being used for tests outside of typed om :(

Do you mind if I took over this bug?
Owner: shend@chromium.org
@shend
sure. 
when i saw result of layout tests after make patch, 
i also worried about some properties which used for test in css-paint-api and css-layout-api.
Project Member

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

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

commit 1ccb08d5469f5bcdc4e1a5256591a17696c958b2
Author: Darren Shen <shend@chromium.org>
Date: Fri Mar 02 01:30:24 2018

[css-typed-om] Whitelist supported properties.

This patch whitelists all our supported properties. Any property that
is not supported will return a base CSSStyleValue. This ensures that
we don't accidentally produce unexpected results for properties that
we haven't tested it.

We also add a few new properties to support, bringing the total number
of property tests to 50. We also remove support for 'z-index' because
it uses <integers> which we don't support yet.

transition-duration is failing tests because we currently convert
ms to seconds when we compute, which is incorrect.

Some paint/layout tests depend on properties that are not whitelisted,
so we've changed them:
- border-radius -> margin-left
- align-items -> empty-cell

Bug:  816402 
Change-Id: I84d2fd8a15df92624122f0c1ecf4f7c42f928928
Reviewed-on: https://chromium-review.googlesource.com/942651
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540397}
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-multi-ref.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-multi.https.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-ref.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map.https.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-background-image-ref.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-background-image.https.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-before-pseudo-ref.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-before-pseudo.https.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-normalization/normalize-numeric.tentative.html
[delete] https://crrev.com/97c6f1f84c9ccf85a328470d437d68b7755655c2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/computed/computed.tentative-expected.txt
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/backface-visibility.html
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/border-collapse.html
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/direction.html
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/empty-cells.html
[rename] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/list-style-image.html
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/overflow-anchor.html
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/resize.html
[add] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/transition-duration-expected.txt
[copy] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/transition-duration.html
[delete] https://crrev.com/97c6f1f84c9ccf85a328470d437d68b7755655c2/third_party/WebKit/LayoutTests/typedcssom/computedstyle/numbers.html
[delete] https://crrev.com/97c6f1f84c9ccf85a328470d437d68b7755655c2/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers.html
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/Source/core/css/CSSProperties.json5
[modify] https://crrev.com/1ccb08d5469f5bcdc4e1a5256591a17696c958b2/third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp

Status: Fixed (was: Started)
Labels: Merge-Request-66
Hi, it looks like #c11 missed the branch cut off. Can I merge this into M66?
Please add affected OSs.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
It's a web platform change that affects all browsers that use Blink.
Status: Assigned (was: Fixed)
Reopening this for the merge.
Project Member

Comment 17 by sheriffbot@chromium.org, Mar 6 2018

Labels: -Merge-Request-66 Merge-Approved-66 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 7 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58

commit 4180e8f8c3f3d51a36578b5aab40a875e0fc8e58
Author: Darren Shen <shend@chromium.org>
Date: Wed Mar 07 00:33:59 2018

[css-typed-om] Whitelist supported properties.

This patch whitelists all our supported properties. Any property that
is not supported will return a base CSSStyleValue. This ensures that
we don't accidentally produce unexpected results for properties that
we haven't tested it.

We also add a few new properties to support, bringing the total number
of property tests to 50. We also remove support for 'z-index' because
it uses <integers> which we don't support yet.

transition-duration is failing tests because we currently convert
ms to seconds when we compute, which is incorrect.

Some paint/layout tests depend on properties that are not whitelisted,
so we've changed them:
- border-radius -> margin-left
- align-items -> empty-cell

Bug:  816402 
Change-Id: I84d2fd8a15df92624122f0c1ecf4f7c42f928928
Reviewed-on: https://chromium-review.googlesource.com/942651
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#540397}(cherry picked from commit 1ccb08d5469f5bcdc4e1a5256591a17696c958b2)
Reviewed-on: https://chromium-review.googlesource.com/952142
Cr-Commit-Position: refs/branch-heads/3359@{#47}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-multi-ref.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-multi.https.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map-ref.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-layout-api/style-map.https.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-background-image-ref.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-background-image.https.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-before-pseudo-ref.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-paint-api/style-before-pseudo.https.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/stylevalue-normalization/normalize-numeric.tentative.html
[delete] https://crrev.com/efc03ea46c5b2e9cf66f723c5213d93ef7441a08/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/computed/computed.tentative-expected.txt
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/backface-visibility.html
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/border-collapse.html
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/direction.html
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/empty-cells.html
[rename] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/list-style-image.html
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/overflow-anchor.html
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/resize.html
[add] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/transition-duration-expected.txt
[copy] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/transition-duration.html
[delete] https://crrev.com/efc03ea46c5b2e9cf66f723c5213d93ef7441a08/third_party/WebKit/LayoutTests/typedcssom/computedstyle/numbers.html
[delete] https://crrev.com/efc03ea46c5b2e9cf66f723c5213d93ef7441a08/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers.html
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/Source/core/css/CSSProperties.json5
[modify] https://crrev.com/4180e8f8c3f3d51a36578b5aab40a875e0fc8e58/third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment