CSS: filter should reject negative brightness |
|||||||||
Issue descriptionhttps://drafts.fxtf.org/filter-effects/#FilterProperty brightness() = brightness( <number-percentage> ) Negative values are not allowed. contrast() = contrast( <number-percentage> ) Negative values are not allowed. Firefox rejects negative values for both. Blink rejects negative contrast but incorrectly accepts negative brightness: FAIL e.style['filter'] = "brightness(-20)" should not set the property value assert_equals: expected "" but got "brightness(-20)" PASS e.style['filter'] = "contrast(-20)" should not set the property value We should add a use counter. I haven't checked the other filter functions.
,
Oct 30 2017
,
Oct 31 2017
Adding this to Hotlist-GoodFirstBug. Small, should be pretty easy, might be able to copy the code for negative contrast.
,
Oct 31 2017
Wait for Q2 2018 when we will have use counter data.
,
Nov 2 2017
Edge (like Firefox) rejects negative values for both. WebKit (like Blink) rejects negative contrast but incorrectly accepts negative brightness:
,
Nov 2 2017
Hello everyone, So do we need to add UseCounter for this or have to check for brightness in the code ? I can check that after your suggestions.
,
Nov 2 2017
Sorry I checked usecounter is already added.
,
Nov 21 2017
I have pushed WIP patch https://chromium-review.googlesource.com/c/chromium/src/+/776647 to ignore negative brightness values. Please give opinion if this will suffice the requirement.
,
Nov 27 2017
Wait for Q2 2018 when we will have use counter data. If the use counts are low, then we should have an Intent to Deprecate/Remove support for negative brightness, and reject negative brightness in the parser. If the use counts are not low, then we contact the CSS WG and discuss changing the spec.
,
Dec 6 2017
,
Feb 4 2018
https://www.chromestatus.com/metrics/feature/timeline/popularity/2193 for future reference, currently at 0.03-0.05% after ~1 week in stable.
,
Feb 4 2018
Web platform test https://wpt.fyi/css/filter-effects/parsing/filter-parsing-invalid.html Blink/WebKit accept negative brightness. Edge and Firefox and spec reject negative brightness.
,
Mar 5 2018
I think we should be good to make this change; use counts are hovering at about 0.01-0.02%. Is anyone who commented above still interested in making this change?
,
Apr 2 2018
,
Apr 5 2018
I put up the Intent here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/eIVnLFD_tPM
,
Apr 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1460d92c07e32ecb026c0187b2c16901cf26e644 commit 1460d92c07e32ecb026c0187b2c16901cf26e644 Author: Rob Buis <rob.buis@samsung.com> Date: Sun Apr 22 17:21:44 2018 Do not accept negative values for brightness filter Do not accept negative values for brightness filter and remove the usecounter for it, which shows low usage (0.01-0.02%). Behavior matches Firefox. Intent to deprecate and remove: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/eIVnLFD_tPM Bug: 776208 Change-Id: I55a49759f96cfd6fbddcca25e8bcbc9046444b26 Reviewed-on: https://chromium-review.googlesource.com/990772 Commit-Queue: Rob Buis <rob.buis@samsung.com> Reviewed-by: Chris Nardi <cnardi@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#552607} [add] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/effect-brightness-clamping-expected.png [rename] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/effect-brightness-clamping-expected.txt [add] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/effect-brightness-expected.png [rename] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/effect-brightness-expected.txt [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/filter-property-parsing-invalid.html [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/css3/filters/filter-property-parsing.html [delete] https://crrev.com/8fc9e80a241c0d8e85f45399e42f9c8154e2ee81/third_party/WebKit/LayoutTests/css3/filters/usecounter-negative-brightness-number.html [delete] https://crrev.com/8fc9e80a241c0d8e85f45399e42f9c8154e2ee81/third_party/WebKit/LayoutTests/css3/filters/usecounter-negative-brightness-percentage.html [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/platform/mac/css3/filters/effect-brightness-clamping-expected.png [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/platform/mac/css3/filters/effect-brightness-clamping-expected.txt [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/platform/mac/css3/filters/effect-brightness-expected.png [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/WebKit/LayoutTests/platform/mac/css3/filters/effect-brightness-expected.txt [delete] https://crrev.com/8fc9e80a241c0d8e85f45399e42f9c8154e2ee81/third_party/WebKit/LayoutTests/platform/win/css3/filters/effect-brightness-clamping-expected.png [delete] https://crrev.com/8fc9e80a241c0d8e85f45399e42f9c8154e2ee81/third_party/WebKit/LayoutTests/platform/win/css3/filters/effect-brightness-expected.png [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/blink/public/platform/web_feature.mojom [modify] https://crrev.com/1460d92c07e32ecb026c0187b2c16901cf26e644/third_party/blink/renderer/core/css/parser/css_property_parser_helpers.cc
,
Apr 23 2018
,
Apr 24 2018
I believe this is all finished?
,
Apr 28 2018
The intent was to deprecate and remove, but instead a patch was landed to remove without deprecation.
,
Apr 28 2018
My impression was that was allowed (in the same vein as intents to implement and ship). https://docs.google.com/document/d/1Z7bbuD5ZMzvvLcUs9kAgYzd65ld80d5-p4Cl2a04bV0/edit seems to suggest that removal can occur immediately if an intent to deprecate and remove is sent, and given no description of a deprecation warning/milestone was mentioned I would assume that was what was thought of.
,
Apr 28 2018
Closing again given #20 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by ericwilligers@chromium.org
, Oct 19 2017