From 94b46be68b2ab131fa6cf178f5b492dce2d5b632 Mon Sep 17 00:00:00 2001 From: violet Date: Mon, 20 May 2019 07:01:29 +0000 Subject: [PATCH] Bug 1520020 - Accept empty argument for some filters r=emilio Filters blur(), invert(), etc. can omit argument. Computed/specified style serialization is a little tricky w.r.t the shortest serialization principle. Ideally we should serialize `invert(1)` to `invert()`, but that will be a breaking change, so we always serialize them with an argument. Note, Blink/WetKit treat specified (but not computed) style serialization differently when the specified one is originally without argument. Our current behavior is the same as pre-Chromium Edge. Differential Revision: https://phabricator.services.mozilla.com/D31720 --HG-- extra : moz-landing-system : lando --- layout/style/test/property_database.js | 18 +++---- .../style/values/specified/effects.rs | 51 +++++++++++++++---- .../filters-test-brightness-003.html.ini | 2 - .../parsing/filter-computed.html.ini | 28 ---------- .../parsing/filter-parsing-valid.html.ini | 31 ----------- .../parsing/filter-computed.html | 4 +- .../parsing/filter-parsing-valid.html | 18 +++---- 7 files changed, 61 insertions(+), 91 deletions(-) delete mode 100644 testing/web-platform/meta/css/filter-effects/filters-test-brightness-003.html.ini delete mode 100644 testing/web-platform/meta/css/filter-effects/parsing/filter-computed.html.ini delete mode 100644 testing/web-platform/meta/css/filter-effects/parsing/filter-parsing-valid.html.ini diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js index 8e4ada7ab803..27e7879ff3a1 100644 --- a/layout/style/test/property_database.js +++ b/layout/style/test/property_database.js @@ -5199,6 +5199,7 @@ var gCSSProperties = { "url('badscheme:badurl')", "blur(3px) url('badscheme:badurl') grayscale(50%)", + "blur()", "blur(0)", "blur(0px)", "blur(0.5px)", @@ -5210,6 +5211,7 @@ var gCSSProperties = { "blur(calc(5px))", "blur(calc(2 * 5px))", + "brightness()", "brightness(0)", "brightness(50%)", "brightness(1)", @@ -5218,6 +5220,7 @@ var gCSSProperties = { "brightness(350%)", "brightness(4.567)", + "contrast()", "contrast(0)", "contrast(50%)", "contrast(1)", @@ -5244,6 +5247,7 @@ var gCSSProperties = { "drop-shadow(calc(2px) calc(2px))", "drop-shadow(calc(2px) calc(2px) calc(2px))", + "grayscale()", "grayscale(0)", "grayscale(50%)", "grayscale(1)", @@ -5252,6 +5256,7 @@ var gCSSProperties = { "grayscale(350%)", "grayscale(4.567)", + "hue-rotate()", "hue-rotate(0)", "hue-rotate(0deg)", "hue-rotate(90deg)", @@ -5263,6 +5268,7 @@ var gCSSProperties = { "hue-rotate(0.5turn)", "hue-rotate(-2turn)", + "invert()", "invert(0)", "invert(50%)", "invert(1)", @@ -5271,6 +5277,7 @@ var gCSSProperties = { "invert(350%)", "invert(4.567)", + "opacity()", "opacity(0)", "opacity(50%)", "opacity(1)", @@ -5279,6 +5286,7 @@ var gCSSProperties = { "opacity(350%)", "opacity(4.567)", + "saturate()", "saturate(0)", "saturate(50%)", "saturate(1)", @@ -5287,6 +5295,7 @@ var gCSSProperties = { "saturate(350%)", "saturate(4.567)", + "sepia()", "sepia(0)", "sepia(50%)", "sepia(1)", @@ -5314,7 +5323,6 @@ var gCSSProperties = { // - Comma delimited arguments // - Wrong argument type // - Argument value out of range - "blur()", "blur(3px 5px)", "blur(3px,)", "blur(3px, 5px)", @@ -5327,7 +5335,6 @@ var gCSSProperties = { "blur(calc(20px - 5%))", "blur(-3px)", - "brightness()", "brightness(0.5 0.5)", "brightness(0.5,)", "brightness(0.5, 0.5)", @@ -5335,7 +5342,6 @@ var gCSSProperties = { "brightness(10px)", "brightness(-1)", - "contrast()", "contrast(0.5 0.5)", "contrast(0.5,)", "contrast(0.5, 0.5)", @@ -5361,7 +5367,6 @@ var gCSSProperties = { "drop-shadow(unset, 2px 2px)", "drop-shadow(2px 2px, unset)", - "grayscale()", "grayscale(0.5 0.5)", "grayscale(0.5,)", "grayscale(0.5, 0.5)", @@ -5369,7 +5374,6 @@ var gCSSProperties = { "grayscale(10px)", "grayscale(-1)", - "hue-rotate()", "hue-rotate(0.5 0.5)", "hue-rotate(0.5,)", "hue-rotate(0.5, 0.5)", @@ -5378,7 +5382,6 @@ var gCSSProperties = { "hue-rotate(-1)", "hue-rotate(45deg,)", - "invert()", "invert(0.5 0.5)", "invert(0.5,)", "invert(0.5, 0.5)", @@ -5386,7 +5389,6 @@ var gCSSProperties = { "invert(10px)", "invert(-1)", - "opacity()", "opacity(0.5 0.5)", "opacity(0.5,)", "opacity(0.5, 0.5)", @@ -5394,7 +5396,6 @@ var gCSSProperties = { "opacity(10px)", "opacity(-1)", - "saturate()", "saturate(0.5 0.5)", "saturate(0.5,)", "saturate(0.5, 0.5)", @@ -5402,7 +5403,6 @@ var gCSSProperties = { "saturate(10px)", "saturate(-1)", - "sepia()", "sepia(0.5 0.5)", "sepia(0.5,)", "sepia(0.5, 0.5)", diff --git a/servo/components/style/values/specified/effects.rs b/servo/components/style/values/specified/effects.rs index d8c378a8a7b2..3ffad2fa89cd 100644 --- a/servo/components/style/values/specified/effects.rs +++ b/servo/components/style/values/specified/effects.rs @@ -17,7 +17,7 @@ use crate::values::specified::color::Color; use crate::values::specified::length::{Length, NonNegativeLength}; #[cfg(feature = "gecko")] use crate::values::specified::url::SpecifiedUrl; -use crate::values::specified::{Angle, NumberOrPercentage}; +use crate::values::specified::{Angle, Number, NumberOrPercentage}; #[cfg(not(feature = "gecko"))] use crate::values::Impossible; use crate::Zero; @@ -62,6 +62,10 @@ impl Factor { }, } } + + fn one() -> Self { + Factor(NumberOrPercentage::Number(Number::new(1.0))) + } } impl Parse for Factor { @@ -209,34 +213,61 @@ impl Parse for Filter { }; input.parse_nested_block(|i| { match_ignore_ascii_case! { &*function, - "blur" => Ok(GenericFilter::Blur((Length::parse_non_negative(context, i)?).into())), - "brightness" => Ok(GenericFilter::Brightness(Factor::parse(context, i)?)), - "contrast" => Ok(GenericFilter::Contrast(Factor::parse(context, i)?)), + "blur" => Ok(GenericFilter::Blur( + i.try(|i| NonNegativeLength::parse(context, i)) + .unwrap_or(Zero::zero()), + )), + "brightness" => Ok(GenericFilter::Brightness( + i.try(|i| Factor::parse(context, i)) + .unwrap_or(Factor::one()), + )), + "contrast" => Ok(GenericFilter::Contrast( + i.try(|i| Factor::parse(context, i)) + .unwrap_or(Factor::one()), + )), "grayscale" => { // Values of amount over 100% are allowed but UAs must clamp the values to 1. // https://drafts.fxtf.org/filter-effects/#funcdef-filter-grayscale - Ok(GenericFilter::Grayscale(Factor::parse_with_clamping_to_one(context, i)?)) + Ok(GenericFilter::Grayscale( + i.try(|i| Factor::parse_with_clamping_to_one(context, i)) + .unwrap_or(Factor::one()), + )) }, "hue-rotate" => { // We allow unitless zero here, see: // https://github.com/w3c/fxtf-drafts/issues/228 - Ok(GenericFilter::HueRotate(Angle::parse_with_unitless(context, i)?)) + Ok(GenericFilter::HueRotate( + i.try(|i| Angle::parse_with_unitless(context, i)) + .unwrap_or(Zero::zero()), + )) }, "invert" => { // Values of amount over 100% are allowed but UAs must clamp the values to 1. // https://drafts.fxtf.org/filter-effects/#funcdef-filter-invert - Ok(GenericFilter::Invert(Factor::parse_with_clamping_to_one(context, i)?)) + Ok(GenericFilter::Invert( + i.try(|i| Factor::parse_with_clamping_to_one(context, i)) + .unwrap_or(Factor::one()), + )) }, "opacity" => { // Values of amount over 100% are allowed but UAs must clamp the values to 1. // https://drafts.fxtf.org/filter-effects/#funcdef-filter-opacity - Ok(GenericFilter::Opacity(Factor::parse_with_clamping_to_one(context, i)?)) + Ok(GenericFilter::Opacity( + i.try(|i| Factor::parse_with_clamping_to_one(context, i)) + .unwrap_or(Factor::one()), + )) }, - "saturate" => Ok(GenericFilter::Saturate(Factor::parse(context, i)?)), + "saturate" => Ok(GenericFilter::Saturate( + i.try(|i| Factor::parse(context, i)) + .unwrap_or(Factor::one()), + )), "sepia" => { // Values of amount over 100% are allowed but UAs must clamp the values to 1. // https://drafts.fxtf.org/filter-effects/#funcdef-filter-sepia - Ok(GenericFilter::Sepia(Factor::parse_with_clamping_to_one(context, i)?)) + Ok(GenericFilter::Sepia( + i.try(|i| Factor::parse_with_clamping_to_one(context, i)) + .unwrap_or(Factor::one()), + )) }, "drop-shadow" => Ok(GenericFilter::DropShadow(Parse::parse(context, i)?)), _ => Err(location.new_custom_error( diff --git a/testing/web-platform/meta/css/filter-effects/filters-test-brightness-003.html.ini b/testing/web-platform/meta/css/filter-effects/filters-test-brightness-003.html.ini deleted file mode 100644 index df5382158af9..000000000000 --- a/testing/web-platform/meta/css/filter-effects/filters-test-brightness-003.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[filters-test-brightness-003.html] - expected: FAIL diff --git a/testing/web-platform/meta/css/filter-effects/parsing/filter-computed.html.ini b/testing/web-platform/meta/css/filter-effects/parsing/filter-computed.html.ini deleted file mode 100644 index 8ca3eda5ba87..000000000000 --- a/testing/web-platform/meta/css/filter-effects/parsing/filter-computed.html.ini +++ /dev/null @@ -1,28 +0,0 @@ -[filter-computed.html] - [Property filter value 'brightness()' computes to 'brightness(0)'] - expected: FAIL - - [Property filter value 'sepia()' computes to 'sepia(1)'] - expected: FAIL - - [Property filter value 'saturate()' computes to 'saturate(1)'] - expected: FAIL - - [Property filter value 'grayscale()' computes to 'grayscale(1)'] - expected: FAIL - - [Property filter value 'invert()' computes to 'invert(0)'] - expected: FAIL - - [Property filter value 'hue-rotate()' computes to 'hue-rotate(0deg)'] - expected: FAIL - - [Property filter value 'blur()' computes to 'blur(0px)'] - expected: FAIL - - [Property filter value 'opacity()' computes to 'opacity(1)'] - expected: FAIL - - [Property filter value 'contrast()' computes to 'contrast(1)'] - expected: FAIL - diff --git a/testing/web-platform/meta/css/filter-effects/parsing/filter-parsing-valid.html.ini b/testing/web-platform/meta/css/filter-effects/parsing/filter-parsing-valid.html.ini deleted file mode 100644 index 1e081c0c745b..000000000000 --- a/testing/web-platform/meta/css/filter-effects/parsing/filter-parsing-valid.html.ini +++ /dev/null @@ -1,31 +0,0 @@ -[filter-parsing-valid.html] - [Serialization should round-trip after setting e.style['filter'\] = "drop-shadow(1px 2px 3px rgba(4, 5, 6, 0.75))"] - expected: FAIL - - [e.style['filter'\] = "saturate()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "grayscale()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "sepia()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "blur()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "contrast()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "invert()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "brightness()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "opacity()" should set the property value] - expected: FAIL - - [e.style['filter'\] = "hue-rotate()" should set the property value] - expected: FAIL - diff --git a/testing/web-platform/tests/css/filter-effects/parsing/filter-computed.html b/testing/web-platform/tests/css/filter-effects/parsing/filter-computed.html index 343e1447f2ea..0c9bb9e19630 100644 --- a/testing/web-platform/tests/css/filter-effects/parsing/filter-computed.html +++ b/testing/web-platform/tests/css/filter-effects/parsing/filter-computed.html @@ -25,7 +25,7 @@ test_computed_value("filter", "blur()", "blur(0px)"); test_computed_value("filter", "brightness(0)"); test_computed_value("filter", "brightness(300%)", "brightness(3)"); -test_computed_value("filter", "brightness()", "brightness(0)"); +test_computed_value("filter", "brightness()", "brightness(1)"); test_computed_value("filter", "contrast(0)"); test_computed_value("filter", "contrast(300%)", "contrast(3)"); @@ -42,7 +42,7 @@ test_computed_value("filter", "hue-rotate()", "hue-rotate(0deg)"); test_computed_value("filter", "invert(0)"); test_computed_value("filter", "invert(100%)", "invert(1)"); -test_computed_value("filter", "invert()", "invert(0)"); +test_computed_value("filter", "invert()", "invert(1)"); test_computed_value("filter", "opacity(0)"); test_computed_value("filter", "opacity(100%)", "opacity(1)"); diff --git a/testing/web-platform/tests/css/filter-effects/parsing/filter-parsing-valid.html b/testing/web-platform/tests/css/filter-effects/parsing/filter-parsing-valid.html index 9d733495cc96..7867c9edae73 100644 --- a/testing/web-platform/tests/css/filter-effects/parsing/filter-parsing-valid.html +++ b/testing/web-platform/tests/css/filter-effects/parsing/filter-parsing-valid.html @@ -16,15 +16,15 @@ test_valid_value("filter", "none"); test_valid_value("filter", "blur(100px)"); test_valid_value("filter", "blur(0)", "blur(0px)"); -test_valid_value("filter", "blur()"); +test_valid_value("filter", "blur()", ["blur()", "blur(0px)"]); test_valid_value("filter", "brightness(0)"); test_valid_value("filter", "brightness(300%)"); -test_valid_value("filter", "brightness()"); +test_valid_value("filter", "brightness()", ["brightness()", "brightness(1)"]); test_valid_value("filter", "contrast(0)"); test_valid_value("filter", "contrast(300%)"); -test_valid_value("filter", "contrast()"); +test_valid_value("filter", "contrast()", ["contrast()", "contrast(1)"]); test_valid_value("filter", "drop-shadow(1px 2px)"); test_valid_value("filter", "drop-shadow(1px 2px 3px)"); @@ -36,27 +36,27 @@ test_valid_value("filter", "drop-shadow(rgba(4, 5, 6, 0.75) 1px 2px 3px)"); test_valid_value("filter", "grayscale(0)"); test_valid_value("filter", "grayscale(300%)", "grayscale(100%)"); -test_valid_value("filter", "grayscale()"); +test_valid_value("filter", "grayscale()", ["grayscale()", "grayscale(1)"]); test_valid_value("filter", "hue-rotate(90deg)"); test_valid_value("filter", "hue-rotate(0)", "hue-rotate(0deg)"); // https://github.com/w3c/fxtf-drafts/issues/228 -test_valid_value("filter", "hue-rotate()"); +test_valid_value("filter", "hue-rotate()", ["hue-rotate()", "hue-rotate(0deg)"]); test_valid_value("filter", "invert(0)"); test_valid_value("filter", "invert(300%)", "invert(100%)"); -test_valid_value("filter", "invert()"); +test_valid_value("filter", "invert()", ["invert()", "invert(1)"]); test_valid_value("filter", "opacity(0)"); test_valid_value("filter", "opacity(300%)", "opacity(100%)"); -test_valid_value("filter", "opacity()"); +test_valid_value("filter", "opacity()", ["opacity()", "opacity(1)"]); test_valid_value("filter", "saturate(0)"); test_valid_value("filter", "saturate(300%)"); -test_valid_value("filter", "saturate()"); +test_valid_value("filter", "saturate()", ["saturate()", "saturate(1)"]); test_valid_value("filter", "sepia(0)"); test_valid_value("filter", "sepia(300%)", "sepia(100%)"); -test_valid_value("filter", "sepia()"); +test_valid_value("filter", "sepia()", ["sepia()", "sepia(1)"]); // Edge serializes url(...) without quotes. Blink/WebKit and Firefox use quotes. test_valid_value("filter", "url(picture.svg#f)", ['url("picture.svg#f")', 'url(picture.svg#f)']);