From f61afa15df7ac16f0d61b5e7db2c68f92d3140a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 24 Mar 2017 20:19:14 +0100 Subject: [PATCH] Bug 1350140: Fix computation of border and outline-width when the border or outline style change. r=heycam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: FjMS47YNNX8 Signed-off-by: Emilio Cobos Álvarez --- components/style/properties/gecko.mako.rs | 92 ++++++++++++++++--- .../style/properties/longhand/outline.mako.rs | 5 +- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index ebfdac88afb..ed6591d6cd6 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -250,7 +250,7 @@ def set_gecko_property(ffi_name, expr): return "self.gecko.%s = %s;" % (ffi_name, expr) %> -<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type='u8')"> +<%def name="impl_keyword_setter(ident, gecko_ffi_name, keyword, cast_type='u8', on_set=None)"> #[allow(non_snake_case)] pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) { use properties::longhands::${ident}::computed_value::T as Keyword; @@ -262,6 +262,9 @@ def set_gecko_property(ffi_name, expr): % endfor }; ${set_gecko_property(gecko_ffi_name, "result")} + % if on_set: + self.${on_set}(); + % endif } @@ -431,17 +434,39 @@ fn color_to_nscolor_zero_currentcolor(color: Color) -> structs::nscolor { } -<%def name="impl_app_units(ident, gecko_ffi_name, need_clone, round_to_pixels=False)"> +<%def name="impl_app_units(ident, gecko_ffi_name, need_clone, inherit_from=None, round_to_pixels=False)"> #[allow(non_snake_case)] pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) { - % if round_to_pixels: - let au_per_device_px = Au(self.gecko.mTwipsPerPixel); - self.gecko.${gecko_ffi_name} = round_border_to_device_pixels(v, au_per_device_px).0; + let value = { + % if round_to_pixels: + let au_per_device_px = Au(self.gecko.mTwipsPerPixel); + round_border_to_device_pixels(v, au_per_device_px).0 + % else: + v.0 + % endif + }; + + % if inherit_from: + self.gecko.${inherit_from} = value; + % endif + self.gecko.${gecko_ffi_name} = value; + } + + #[allow(non_snake_case)] + pub fn copy_${ident}_from(&mut self, other: &Self) { + % if inherit_from: + self.gecko.${inherit_from} = other.gecko.${inherit_from}; + // NOTE: This is needed to easily handle the `unset` and `initial` + // keywords, which are implemented calling this function. + // + // In practice, this means that we may have an incorrect value here, but + // we'll adjust that properly in the style fixup phase. + self.gecko.${gecko_ffi_name} = other.gecko.${inherit_from}; % else: - self.gecko.${gecko_ffi_name} = v.0; + self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name}; % endif } -<%call expr="impl_simple_copy(ident, gecko_ffi_name)"> + %if need_clone: #[allow(non_snake_case)] pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T { @@ -794,12 +819,48 @@ fn static_assert() { skip_additionals="*"> % for side in SIDES: - <% impl_keyword("border_%s_style" % side.ident, "mBorderStyle[%s]" % side.index, border_style_keyword, + <% impl_keyword("border_%s_style" % side.ident, + "mBorderStyle[%s]" % side.index, + border_style_keyword, + on_set="update_border_%s" % side.ident, need_clone=True) %> + // This is needed because the initial mComputedBorder value is set to zero. + // + // In order to compute stuff, we start from the initial struct, and keep + // going down the tree applying properties. + // + // That means, effectively, that when we set border-style to something + // non-hidden, we should use the initial border instead. + // + // Servo stores the initial border-width in the initial struct, and then + // adjusts as needed in the fixup phase. This means that the initial struct + // is technically not valid without fixups, and that you lose pretty much + // any sharing of the initial struct, which is kind of unfortunate. + // + // Gecko has two fields for this, one that stores the "specified" border, + // and other that stores the actual computed one. That means that when we + // set border-style, border-width may change and we need to sync back to the + // specified one. This is what this function does. + // + // Note that this doesn't impose any dependency in the order of computation + // of the properties. This is only relevant if border-style is specified, + // but border-width isn't. If border-width is specified at some point, the + // two mBorder and mComputedBorder fields would be the same already. + // + // Once we're here, we know that we'll run style fixups, so it's fine to + // just copy the specified border here, we'll adjust it if it's incorrect + // later. + fn update_border_${side.ident}(&mut self) { + self.gecko.mComputedBorder.${side.ident} = self.gecko.mBorder.${side.ident}; + } + <% impl_color("border_%s_color" % side.ident, "(mBorderColor)[%s]" % side.index, need_clone=True) %> - <% impl_app_units("border_%s_width" % side.ident, "mComputedBorder.%s" % side.ident, need_clone=True, + <% impl_app_units("border_%s_width" % side.ident, + "mComputedBorder.%s" % side.ident, + inherit_from="mBorder.%s" % side.ident, + need_clone=True, round_to_pixels=True) %> pub fn border_${side.ident}_has_nonzero_width(&self) -> bool { @@ -1130,7 +1191,8 @@ fn static_assert() { #[allow(non_snake_case)] pub fn set_outline_style(&mut self, v: longhands::outline_style::computed_value::T) { - // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts + // FIXME(bholley): Align binary representations and ditch |match| for + // cast + static_asserts let result = match v { % for value in border_style_keyword.values_for('gecko'): Either::Second(border_style::T::${to_rust_ident(value)}) => @@ -1140,6 +1202,11 @@ fn static_assert() { structs::${border_style_keyword.gecko_constant('auto')} ${border_style_keyword.maybe_cast("u8")}, }; ${set_gecko_property("mOutlineStyle", "result")} + + // NB: This is needed to correctly handling the initial value of + // outline-width when outline-style changes, see the + // update_border_${side} comment for more details. + self.gecko.mActualOutlineWidth = self.gecko.mOutlineWidth; } #[allow(non_snake_case)] @@ -1161,8 +1228,9 @@ fn static_assert() { } } - <% impl_app_units("outline_width", "mActualOutlineWidth", need_clone=True, - round_to_pixels=True) %> + <% impl_app_units("outline_width", "mActualOutlineWidth", + inherit_from="mOutlineWidth", + need_clone=True, round_to_pixels=True) %> % for corner in CORNERS: <% impl_corner_style_coord("_moz_outline_radius_%s" % corner.ident.replace("_", ""), diff --git a/components/style/properties/longhand/outline.mako.rs b/components/style/properties/longhand/outline.mako.rs index 8372943bc81..97f3363ad15 100644 --- a/components/style/properties/longhand/outline.mako.rs +++ b/components/style/properties/longhand/outline.mako.rs @@ -53,8 +53,9 @@ ${helpers.predefined_type("outline-color", "CSSColor", "computed::CSSColor::Curr SpecifiedValue::parse(context, input) .and_then(|result| { if let Either::Second(BorderStyle::hidden) = result { - // The outline-style property accepts the same values as border-style, - // except that 'hidden' is not a legal outline style. + // The outline-style property accepts the same values as + // border-style, except that 'hidden' is not a legal outline + // style. Err(()) } else { Ok(result)