New issue
Advanced search Search tips

Issue 848698 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 641877



Sign in to add a comment

CSS.registerProperty: detect depedency cycles from length units

Project Member Reported by andruud@chromium.org, Jun 1 2018

Issue description

For registered properties, the following should not be valid:

  --font-size: 10em;
  font-size: var(--font-size);

When resolving the value of font-size, we need to detect that the var() has a cycle, and discard the declaration.

The same goes for other font dependent units.

https://github.com/w3c/css-houdini-drafts/issues/315

 
regprop3.html
308 bytes View Download
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 10

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

commit fcc2b1720620f0004b5f34d4468f708bf76d5799
Author: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue Jul 10 09:10:13 2018

Use a single Result object when resolving custom variable references.

New result flags are about to be added. This refactor makes that patch
less noisy.

Bug:  848698 
Change-Id: I7e7495ba2265a7ffdc329bab59e772c4ed5e65ff
Reviewed-on: https://chromium-review.googlesource.com/1120329
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Anders Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573656}
[modify] https://crrev.com/fcc2b1720620f0004b5f34d4468f708bf76d5799/third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc
[modify] https://crrev.com/fcc2b1720620f0004b5f34d4468f708bf76d5799/third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 10

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

commit 58da3151773fb9b5d8607b540c2a01fc05a1e6f8
Author: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue Jul 10 09:15:36 2018

Use an Options object to propagate flags when resolving var-references.

Two new flags will be added soon. This patch makes that future diff
cleaner.

Note: The reason the Options object is private is that the two new flags
      I had in mind can be set in ResolveVariableReferences from the
      incoming CSSPropertyID and the StyleResolverState.

Bug:  848698 
Change-Id: I644681d0a199e4b2727d550f51535f3e140be44a
Reviewed-on: https://chromium-review.googlesource.com/1120330
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573657}
[modify] https://crrev.com/58da3151773fb9b5d8607b540c2a01fc05a1e6f8/third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc
[modify] https://crrev.com/58da3151773fb9b5d8607b540c2a01fc05a1e6f8/third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 10

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

commit c13a893ebf9a176c754209b23955a471d0313afb
Author: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue Jul 10 11:52:23 2018

Detect font units in CSSVariableData.

The CSSWG has decided that the following is not permissible, for a
registered custom property --foo:

   --foo: 10em;
   font-size: var(--foo);

This is because --foo depends on font-size via the 'em' unit, and
font-size depends on --foo via the var reference.

In order to detect this situation, we have to know whether a certain
CSSVariableData contains font units or not. This patch adds that
capability, however, it is currently unused information. A subsequent
patch will implement the actual check.

Bug:  848698 
Change-Id: I7861a089afc89c4fe7b22ad4f056f92e6248978a
Reviewed-on: https://chromium-review.googlesource.com/1120331
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573685}
[modify] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/css/css_variable_data.cc
[modify] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/css/css_variable_data.h
[add] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/css/resolver/css_variable_data_test.cc
[modify] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc
[modify] https://crrev.com/c13a893ebf9a176c754209b23955a471d0313afb/third_party/blink/renderer/core/css/style_environment_variables.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 10

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

commit 4ab1f973716a535b39e6f10843a992e1a1ec26ae
Author: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue Jul 10 13:59:32 2018

Disallow font-relative units when resolving var() for font-size.

For a registered custom property --foo, the following is no longer
allowed:

   --foo: 10em;
   font-size: var(--foo);

The same applies to ex and ch, as well as rem on the root element.

This is because --foo depends on font-size via the 'em' unit, and
font-size depends on --foo via the var() reference. This creates a
circular dependency that must be resolved somehow. The agreed-upon way
to resolve that, has been by treating the whole var() reference as
invalid.

This patch implements that cycle detection by adding flags to disallow
resolution of any var() references that contain font-relative units.
These flags are set in ResolveVariableReferences based on the
CSSPropertyID/StyleResolverState.

When a custom property itself contains var() references, it needs to
"inherit" the font-unit-ness from the resolved variables (similar to
is_animation_tainted). This is why flags have also been added to Result.

Bug:  848698 
Change-Id: Ife0570949d996c5fc26dc10b1d7ca15bd27343b1
Reviewed-on: https://chromium-review.googlesource.com/1120334
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573712}
[modify] https://crrev.com/4ab1f973716a535b39e6f10843a992e1a1ec26ae/third_party/WebKit/LayoutTests/external/wpt/css/css-properties-values-api/registered-property-computation.html
[add] https://crrev.com/4ab1f973716a535b39e6f10843a992e1a1ec26ae/third_party/WebKit/LayoutTests/external/wpt/css/css-properties-values-api/unit-cycles.html
[modify] https://crrev.com/4ab1f973716a535b39e6f10843a992e1a1ec26ae/third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc
[modify] https://crrev.com/4ab1f973716a535b39e6f10843a992e1a1ec26ae/third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

Status: Fixed (was: Started)

Sign in to add a comment