New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 776208 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocked on:
issue 776276



Sign in to add a comment

CSS: filter should reject negative brightness

Project Member Reported by ericwilligers@chromium.org, Oct 19 2017

Issue description

https://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.

 
Blockedon: 776276
Labels: Code-Parser
Labels: Hotlist-GoodFirstBug
Adding this to Hotlist-GoodFirstBug. Small, should be pretty easy, might be able to copy the code for negative contrast.
Wait for Q2 2018 when we will have use counter data.
Edge (like Firefox) rejects negative values for both. WebKit (like Blink) rejects negative contrast but incorrectly accepts negative brightness:


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.
Sorry I checked usecounter is already added.
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.
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.

Labels: -Update-Quarterly
https://www.chromestatus.com/metrics/feature/timeline/popularity/2193 for future reference, currently at 0.03-0.05% after ~1 week in stable.
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.

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?
Owner: rob.b...@samsung.com
Status: Assigned (was: Available)
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Owner: rob.buis@chromium.org
Status: Fixed (was: Assigned)
I believe this is all finished?
Cc: futhark@chromium.org
Status: Started (was: Fixed)
The intent was to deprecate and remove, but instead a patch was landed to remove without deprecation.
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.
Status: Fixed (was: Started)
Closing again given #20

Sign in to add a comment