New issue
Advanced search Search tips

Issue 814606 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] Handle variable references correctly for list-valued properties.

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

Issue description

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

This might be a bit difficult to do as Blink currently treats something like 'var(--A), var(--b)' as a single CSSVariableReferenceValue, not two. To handle this properly, we'd probably have to do some tokenization ourselves to find all the vars. Furthermore, it looks like there are properties that can take both lists & single values (e.g. backdrop-filter), so not sure what should happen there.

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

For now, we just reject when we set with more than one variable reference (e.g. styleMap.set('transition-duration', '1s', 'var(--A)').

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

New resolution: We just reject when there's more than one variable reference value (same as current behaviour). We also need to reject when appending.
Project Member

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

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

commit 5014f339d1c7734cf21cde33780859515f460519
Author: Darren Shen <shend@chromium.org>
Date: Mon Mar 05 21:17:35 2018

[css-typed-om] Fix crash when appending var refs.

We currently hit a DCHECK when appending a var ref. This patch fixes
the crash by rejecting it as per the spec.

Spec: https://github.com/w3c/css-houdini-drafts/commit/ecfcbf9721c0d7dff7890d8e009e4e3ad9b72d10

Bug:  814606 
Change-Id: I144307bf489db5e968c8e326087d13554ec3e59c
Reviewed-on: https://chromium-review.googlesource.com/948142
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540938}
[modify] https://crrev.com/5014f339d1c7734cf21cde33780859515f460519/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/append.tentative.html
[modify] https://crrev.com/5014f339d1c7734cf21cde33780859515f460519/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/set.tentative.html
[modify] https://crrev.com/5014f339d1c7734cf21cde33780859515f460519/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/append.tentative.html
[modify] https://crrev.com/5014f339d1c7734cf21cde33780859515f460519/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/set.tentative.html
[modify] https://crrev.com/5014f339d1c7734cf21cde33780859515f460519/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp

Comment 5 by shend@chromium.org, Mar 5 2018

Status: Fixed (was: Assigned)

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

Labels: Merge-Request-66 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Status: Assigned (was: Fixed)
Requesting merge for M66. Thanks!

Comment 7 by shend@chromium.org, Mar 7 2018

Sorry, to be clear, the patch is the one in #c4.
Project Member

Comment 8 by sheriffbot@chromium.org, Mar 8 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 9 by bugdroid1@chromium.org, Mar 9 2018

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

commit d4f22c85e7ae98d052793c328a88d8d19b344236
Author: Darren Shen <shend@chromium.org>
Date: Fri Mar 09 03:26:58 2018

[css-typed-om] Fix crash when appending var refs.

We currently hit a DCHECK when appending a var ref. This patch fixes
the crash by rejecting it as per the spec.

Spec: https://github.com/w3c/css-houdini-drafts/commit/ecfcbf9721c0d7dff7890d8e009e4e3ad9b72d10

Bug:  814606 
Change-Id: I144307bf489db5e968c8e326087d13554ec3e59c
Reviewed-on: https://chromium-review.googlesource.com/948142
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#540938}(cherry picked from commit 5014f339d1c7734cf21cde33780859515f460519)
Reviewed-on: https://chromium-review.googlesource.com/956382
Cr-Commit-Position: refs/branch-heads/3359@{#130}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/d4f22c85e7ae98d052793c328a88d8d19b344236/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/append.tentative.html
[modify] https://crrev.com/d4f22c85e7ae98d052793c328a88d8d19b344236/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/declared/set.tentative.html
[modify] https://crrev.com/d4f22c85e7ae98d052793c328a88d8d19b344236/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/append.tentative.html
[modify] https://crrev.com/d4f22c85e7ae98d052793c328a88d8d19b344236/third_party/WebKit/LayoutTests/external/wpt/css/css-typed-om/the-stylepropertymap/inline/set.tentative.html
[modify] https://crrev.com/d4f22c85e7ae98d052793c328a88d8d19b344236/third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment