Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issue 2406203004: Compute registered properties after resolving high priority properties (Closed)

Created:
4 years, 2 months ago by Timothy Loh
Modified:
4 years, 2 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Compute registered properties after resolving high priority properties This patch moves computation of registered properties after that of high priority properties. This makes em units work correctly when the font-size is set on a given element. Currently properties which are inherited will be re-computed since we don't have a distinction between inherited properties and those which we actually set. This does not affect correctness as computation is idempotent. As we resolve var() references as token streams, we don't issues with cycles (e.g. --font-size: 10em; font-size: var(--font-size)). There's an outstanding issue on the Houdini GitHub to work out what we should actually do here. https://github.com/w3c/css-houdini-drafts/issues/315 BUG=641877 Committed: https://crrev.com/d334ba8f91d9dc54d00145dbd0deffc3f32e283f Cr-Commit-Position: refs/heads/master@{#424697}

Patch Set 1 #

Total comments: 1

Patch Set 2 : font size cycle test #

Messages

Total messages: 17 (10 generated)
Timothy Loh
4 years, 2 months ago (2016-10-12 05:22:00 UTC) #5
alancutter (OOO until 2018)
lgtm We should have a test that --x: 1em; font-size: var(--x) compute to different values. ...
4 years, 2 months ago (2016-10-12 06:49:28 UTC) #8
Timothy Loh
On 2016/10/12 06:49:28, alancutter wrote: > lgtm > We should have a test that --x: ...
4 years, 2 months ago (2016-10-12 06:51:09 UTC) #9
alancutter (OOO until 2018)
On 2016/10/12 at 06:51:09, timloh wrote: > On 2016/10/12 06:49:28, alancutter wrote: > > lgtm ...
4 years, 2 months ago (2016-10-12 07:18:28 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2406203004/20001
4 years, 2 months ago (2016-10-12 07:24:57 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 2 months ago (2016-10-12 09:21:57 UTC) #15
commit-bot: I haz the power
4 years, 2 months ago (2016-10-12 09:24:09 UTC) #17
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/d334ba8f91d9dc54d00145dbd0deffc3f32e283f
Cr-Commit-Position: refs/heads/master@{#424697}

Powered by Google App Engine
This is Rietveld 408576698