CSSParser::parseSingleValue is inconsistent in handling css-wide keywords |
|||||||
Issue descriptionCSSParser::parseSingleValue first calls the fast path, and if the fast path fails it calls into CSSPropertyParser::parseSingleValue. The fast path supports the strings "initial" and "inherit" but the slow path does *not* support any css-wide keywords. So CSSParser::parseSingleValue is allowing the strings "initial"/"inherit" but not the other css-wide keywords, or usage of escapes. From a brief look at the callers it seems like everything ends up handling it correctly, but we should fix this.
,
Jun 20 2017
Hey Bugs! it's this again! Perhaps something to consider with Ribbon.
,
Jun 30 2017
,
Sep 12 2017
,
Sep 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/778279c9d06879103f0f0698849eff541204e159 commit 778279c9d06879103f0f0698849eff541204e159 Author: Rob Buis <rob.buis@samsung.com> Date: Wed Sep 13 15:47:24 2017 Support unset in CSSParserFastPaths Support unset as a CSS wide keyword [1] and add some unit tests for it. [1] https://www.w3.org/TR/css3-values/#common-keywords Bug: 660273 Change-Id: Ic739055b83988017e11ff3968c1a0aad48073613 Reviewed-on: https://chromium-review.googlesource.com/663820 Commit-Queue: Rob Buis <rob.buis@samsung.com> Reviewed-by: Bugs Nash <bugsnash@chromium.org> Reviewed-by: meade_UTC10 <meade@chromium.org> Cr-Commit-Position: refs/heads/master@{#501642} [modify] https://crrev.com/778279c9d06879103f0f0698849eff541204e159/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp [modify] https://crrev.com/778279c9d06879103f0f0698849eff541204e159/third_party/WebKit/Source/core/css/parser/CSSParserFastPathsTest.cpp
,
Sep 13 2017
I wonder if anything is left for this bug? I am pretty sure the slow path does support css-wide keywords these days through ConsumeCSSWideKeyword. If so, the only thing I am not sure about is the "usage of escapes" comment.
,
Sep 14 2017
I think this is still an issue, let me try to clarify. I think your patch would've changed how BaseRenderingContext2D::setFilter() handles unset and maybe it's not correct now?
The call tree (for example) looks like this:
BaseRenderingContext2D::setFilter()
-> CSSParser::ParseSingleValue
-1-> CSSParserFastPaths::MaybeParseValue (handles "initial", "inherit", "unset")
-2-> CSSPropertyParser::ParseSingleValue (handles "blur(10px)", not "initial")
In the CSSPropertyParser, CSS-wide keywords are handled in ParseValueStart. So CSSParser::ParseSingleValue() will allow those handled by the fast-path (e.g. "initial"), but not cases it misses like "initi\61 l". So the API is somewhat broken because it seems to support css-wide keywords but if escapes are used it will reject.
So we need to decide on whether this function will support or reject css-wide keywords (no opinion here since I haven't worked on or looked at style code since January). I think all the callers used to explicitly rejected them, but it seems like CSSStyleValue.parse now wants to accept them:
> CSS.supports("filter", "initial")
true
> CSS.supports("filter", "initi\\61 l")
true
> CSSStyleValue.parse("filter", "initial")
CSSKeywordValue {keywordValue: "initial", cssText: "initial"}
> CSSStyleValue.parse("filter", "initi\\61 l")
null
,
Sep 14 2017
Thanks Tim for the explanation!
>I think this is still an issue, let me try to clarify. I think your patch would've changed how BaseRenderingContext2D::setFilter() handles unset and maybe it's not correct now?
Yes from messing around with inspector it behaves differently from initial/inherit when setting the filter attribute. The callsite should just use IsCSSWideKeyword, working on a patch.
>The call tree (for example) looks like this:
>BaseRenderingContext2D::setFilter()
> -> CSSParser::ParseSingleValue
> -1-> CSSParserFastPaths::MaybeParseValue (handles "initial", "inherit", "unset")
> -2-> CSSPropertyParser::ParseSingleValue (handles "blur(10px)", not "initial")
> In the CSSPropertyParser, CSS-wide keywords are handled in ParseValueStart. So CSSParser::ParseSingleValue() will allow those handled by the fast-path (e.g. "initial"), but not cases it misses like
> "initi\61 l". So the API is somewhat broken because it seems to support css-wide keywords but if escapes are used it will reject.
>So we need to decide on whether this function will support or reject css-wide keywords (no opinion here since I haven't worked on or looked at style code since January). I think all the callers used to >explicitly rejected them, but it seems like CSSStyleValue.parse now wants to accept them:
> CSS.supports("filter", "initial")
true
> CSS.supports("filter", "initi\\61 l")
true
> CSSStyleValue.parse("filter", "initial")
CSSKeywordValue {keywordValue: "initial", cssText: "initial"}
> CSSStyleValue.parse("filter", "initi\\61 l")
null
Sounds to me like it would be useful to know how CSSStyleValue.parse should behave in above case. Should I write the mailing list or does somebody else volunteer? :)
,
Sep 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c9be53b404ca9b9386200357003d8037005f804 commit 5c9be53b404ca9b9386200357003d8037005f804 Author: yiyix <yiyix@chromium.org> Date: Mon Sep 18 09:23:39 2017 Simplify logic in UIResourceLayerImpl::AppendQuad In UIResourceLayerImpl::AppendQuad, we are always retrieving resource Id from UIResourceId regardless of validation of UIResourceId. I added a validation check prior of calling ResourceIdForUIResource in this cl. Bug: 660273 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Idda682cef70823520300e2842de1f488932bd285 Reviewed-on: https://chromium-review.googlesource.com/664960 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Yi Xu <yiyix@chromium.org> Cr-Commit-Position: refs/heads/master@{#502540} [modify] https://crrev.com/5c9be53b404ca9b9386200357003d8037005f804/cc/layers/ui_resource_layer_impl.cc
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2491a1d2ded89d2cc3f52856524411042488139a commit 2491a1d2ded89d2cc3f52856524411042488139a Author: Rob Buis <rob.buis@samsung.com> Date: Tue Sep 19 13:51:45 2017 Fix callsites of CSSParser::ParseSingleValue After #501642 some callsites behave differently because they only tested for initial/inherit, not unset. So use IsCSSWideKeyword instead. Bug: 660273 Change-Id: Ibe8842ac21da804c43104fd978983d84c9f65001 Reviewed-on: https://chromium-review.googlesource.com/667865 Reviewed-by: Timothy Loh <timloh@chromium.org> Reviewed-by: Bugs Nash <bugsnash@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Rob Buis <rob.buis@samsung.com> Cr-Commit-Position: refs/heads/master@{#502843} [modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/LayoutTests/fast/canvas/canvas-filter-value.html [modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/Source/core/html/canvas/CanvasFontCache.cpp [modify] https://crrev.com/2491a1d2ded89d2cc3f52856524411042488139a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
,
Dec 6 2017
,
Apr 23 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bugsnash@chromium.org
, Feb 13 2017