From f195df79f39b76629e27c9f76b76cd950fb52f6a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 6 May 2014 16:52:53 +0100 Subject: [PATCH 1/8] Make a comment more precise about derived properties. --- src/components/style/properties.rs.mako | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 8f3fe933daa..4f6b8830ff0 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1582,7 +1582,7 @@ fn cascade_with_cached_declarations(applicable_declarations: &[MatchedProperty], } % else: ${property.ident}_declaration(_) => { - // Ignore derived properties; they cannot be set by content. + // Do not allow stylesheets to set derived properties. } % endif % endfor @@ -1777,7 +1777,7 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], } % else: ${property.ident}_declaration(_) => { - // Ignore derived properties; they cannot be set by content. + // Do not allow stylesheets to set derived properties. } % endif % endfor From 8b53ea8a2cd8b02095da0986df428d45d6a76152 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 6 May 2014 16:56:37 +0100 Subject: [PATCH 2/8] Cascade declarations in reverse order, skipping those already seen Previously we processed them in forward order, latter dcelarations for the same property overriding any earlier one, making the work of converting the earlier ones to a computed value redundant. Maintaining a bit field of "seen" properties will also help fixing #2288. --- src/components/style/properties.rs.mako | 68 +++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 4f6b8830ff0..eecb4fde494 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -7,6 +7,7 @@ #![allow(non_camel_case_types, uppercase_variables)] pub use std::ascii::StrAsciiExt; +use serialize::{Encodable, Encoder}; pub use servo_util::url::parse_url; use sync::Arc; @@ -21,10 +22,11 @@ pub use parsing_utils::*; pub use self::common_types::*; use selector_matching::MatchedProperty; -use serialize::{Encodable, Encoder}; +pub use self::property_bit_field::PropertyBitField; pub mod common_types; + <%! def to_rust_ident(name): @@ -1331,6 +1333,52 @@ pub mod shorthands { } +// TODO(SimonSapin): Convert this to a syntax extension rather than a Mako template. +// Maybe submit for inclusion in libstd? +mod property_bit_field { + use std::uint; + use std::mem; + + pub struct PropertyBitField { + storage: [uint, ..(${len(LONGHANDS)} - 1 + uint::BITS) / uint::BITS] + } + + impl PropertyBitField { + #[inline] + pub fn new() -> PropertyBitField { + PropertyBitField { storage: unsafe { mem::init() } } + } + + #[inline] + fn get(&self, bit: uint) -> bool { + (self.storage[bit / uint::BITS] & (1 << (bit % uint::BITS))) != 0 + } + #[inline] + fn set(&mut self, bit: uint) { + self.storage[bit / uint::BITS] |= 1 << (bit % uint::BITS) + } + #[inline] + fn clear(&mut self, bit: uint) { + self.storage[bit / uint::BITS] &= !(1 << (bit % uint::BITS)) + } + % for i, property in enumerate(LONGHANDS): + #[inline] + pub fn get_${property.ident}(&self) -> bool { + self.get(${i}) + } + #[inline] + pub fn set_${property.ident}(&mut self) { + self.set(${i}) + } + #[inline] + pub fn clear_${property.ident}(&mut self) { + self.clear(${i}) + } + % endfor + } +} + + pub struct PropertyDeclarationBlock { pub important: Arc>, pub normal: Arc>, @@ -1539,14 +1587,19 @@ fn cascade_with_cached_declarations(applicable_declarations: &[MatchedProperty], % endif % endfor - for sub_list in applicable_declarations.iter() { - for declaration in sub_list.declarations.iter() { + let mut seen = PropertyBitField::new(); + for sub_list in applicable_declarations.iter().rev() { + for declaration in sub_list.declarations.iter().rev() { match *declaration { % for style_struct in STYLE_STRUCTS: % if style_struct.inherited: % for property in style_struct.longhands: % if property.derived_from is None: ${property.ident}_declaration(ref declared_value) => { + if seen.get_${property.ident}() { + continue + } + seen.set_${property.ident}(); let computed_value = match *declared_value { SpecifiedValue(ref specified_value) => longhands::${property.ident}::to_computed_value( @@ -1734,13 +1787,18 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], .${style_struct.name}.clone(); % endfor let mut cacheable = true; - for sub_list in applicable_declarations.iter() { - for declaration in sub_list.declarations.iter() { + let mut seen = PropertyBitField::new(); + for sub_list in applicable_declarations.iter().rev() { + for declaration in sub_list.declarations.iter().rev() { match *declaration { % for style_struct in STYLE_STRUCTS: % for property in style_struct.longhands: % if property.derived_from is None: ${property.ident}_declaration(ref declared_value) => { + if seen.get_${property.ident}() { + continue + } + seen.set_${property.ident}(); let computed_value = match *declared_value { SpecifiedValue(ref specified_value) => longhands::${property.ident}::to_computed_value( From 416cbf472a595ca4a641a9d6f9db52c52b987e10 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 6 May 2014 18:31:38 +0100 Subject: [PATCH 3/8] Parse declarations in reverse order, skip those that would be overridden. --- src/components/style/properties.rs.mako | 124 +++++++++++++++--------- 1 file changed, 79 insertions(+), 45 deletions(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index eecb4fde494..00e572f6d64 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1379,6 +1379,8 @@ mod property_bit_field { } +/// Declarations are stored in reverse order. +/// Overridden declarations are skipped. pub struct PropertyDeclarationBlock { pub important: Arc>, pub normal: Arc>, @@ -1397,28 +1399,36 @@ pub fn parse_style_attribute(input: &str, base_url: &Url) -> PropertyDeclaration pub fn parse_property_declaration_list>(input: I, base_url: &Url) -> PropertyDeclarationBlock { - let mut important = vec!(); - let mut normal = vec!(); - for item in ErrorLoggerIterator(parse_declaration_list(input)) { + let mut important_declarations = vec!(); + let mut normal_declarations = vec!(); + let mut important_seen = PropertyBitField::new(); + let mut normal_seen = PropertyBitField::new(); + let items: Vec = + ErrorLoggerIterator(parse_declaration_list(input)).collect(); + for item in items.move_iter().rev() { match item { DeclAtRule(rule) => log_css_error( rule.location, format!("Unsupported at-rule in declaration list: @{:s}", rule.name)), Declaration(Declaration{ location: l, name: n, value: v, important: i}) => { // TODO: only keep the last valid declaration for a given name. - let list = if i { &mut important } else { &mut normal }; - match PropertyDeclaration::parse(n, v, list, base_url) { + let (list, seen) = if i { + (&mut important_declarations, &mut important_seen) + } else { + (&mut normal_declarations, &mut normal_seen) + }; + match PropertyDeclaration::parse(n, v, list, base_url, seen) { UnknownProperty => log_css_error(l, format!( "Unsupported property: {}:{}", n, v.iter().to_css())), InvalidValue => log_css_error(l, format!( "Invalid value: {}:{}", n, v.iter().to_css())), - ValidDeclaration => (), + ValidOrIgnoredDeclaration => (), } } } } PropertyDeclarationBlock { - important: Arc::new(important), - normal: Arc::new(normal), + important: Arc::new(important_declarations), + normal: Arc::new(normal_declarations), } } @@ -1460,64 +1470,88 @@ pub enum PropertyDeclaration { pub enum PropertyDeclarationParseResult { UnknownProperty, InvalidValue, - ValidDeclaration, + ValidOrIgnoredDeclaration, } impl PropertyDeclaration { pub fn parse(name: &str, value: &[ComponentValue], result_list: &mut Vec, - base_url: &Url) -> PropertyDeclarationParseResult { + base_url: &Url, + seen: &mut PropertyBitField) -> PropertyDeclarationParseResult { // FIXME: local variable to work around Rust #10683 let name_lower = name.to_ascii_lower(); match name_lower.as_slice() { % for property in LONGHANDS: % if property.derived_from is None: - "${property.name}" => result_list.push(${property.ident}_declaration( - match longhands::${property.ident}::parse_declared(value, base_url) { - Some(value) => value, - None => return InvalidValue, + "${property.name}" => { + if seen.get_${property.ident}() { + return ValidOrIgnoredDeclaration } - )), + match longhands::${property.ident}::parse_declared(value, base_url) { + Some(value) => { + seen.set_${property.ident}(); + result_list.push(${property.ident}_declaration(value)); + ValidOrIgnoredDeclaration + }, + None => InvalidValue, + } + }, % else: - "${property.name}" => {} + "${property.name}" => UnknownProperty, % endif % endfor % for shorthand in SHORTHANDS: - "${shorthand.name}" => match CSSWideKeyword::parse(value) { - Some(Some(keyword)) => { - % for sub_property in shorthand.sub_properties: - result_list.push(${sub_property.ident}_declaration( - CSSWideKeyword(keyword) - )); - % endfor - }, - Some(None) => { - % for sub_property in shorthand.sub_properties: - result_list.push(${sub_property.ident}_declaration( - CSSWideKeyword(${ - "Inherit" if sub_property.style_struct.inherited else "Initial"}) - )); - % endfor - }, - None => match shorthands::${shorthand.ident}::parse(value, base_url) { - Some(result) => { + "${shorthand.name}" => { + if ${" && ".join("seen.get_%s()" % sub_property.ident + for sub_property in shorthand.sub_properties)} { + return ValidOrIgnoredDeclaration + } + match CSSWideKeyword::parse(value) { + Some(Some(keyword)) => { % for sub_property in shorthand.sub_properties: - result_list.push(${sub_property.ident}_declaration( - match result.${sub_property.ident} { - Some(value) => SpecifiedValue(value), - None => CSSWideKeyword(Initial), - } - )); + if !seen.get_${sub_property.ident}() { + seen.set_${sub_property.ident}(); + result_list.push(${sub_property.ident}_declaration( + CSSWideKeyword(keyword))); + } % endfor + ValidOrIgnoredDeclaration }, - None => return InvalidValue, + Some(None) => { + % for sub_property in shorthand.sub_properties: + if !seen.get_${sub_property.ident}() { + seen.set_${sub_property.ident}(); + result_list.push(${sub_property.ident}_declaration( + CSSWideKeyword(${ + "Inherit" if sub_property.style_struct.inherited else "Initial" + }))); + } + % endfor + ValidOrIgnoredDeclaration + }, + None => match shorthands::${shorthand.ident}::parse(value, base_url) { + Some(result) => { + % for sub_property in shorthand.sub_properties: + if !seen.get_${sub_property.ident}() { + seen.set_${sub_property.ident}(); + result_list.push(${sub_property.ident}_declaration( + match result.${sub_property.ident} { + Some(value) => SpecifiedValue(value), + None => CSSWideKeyword(Initial), + } + )); + } + % endfor + ValidOrIgnoredDeclaration + }, + None => InvalidValue, + } } }, % endfor - _ => return UnknownProperty, + _ => UnknownProperty, } - ValidDeclaration } } @@ -1589,7 +1623,7 @@ fn cascade_with_cached_declarations(applicable_declarations: &[MatchedProperty], let mut seen = PropertyBitField::new(); for sub_list in applicable_declarations.iter().rev() { - for declaration in sub_list.declarations.iter().rev() { + for declaration in sub_list.declarations.iter() { match *declaration { % for style_struct in STYLE_STRUCTS: % if style_struct.inherited: @@ -1789,7 +1823,7 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], let mut cacheable = true; let mut seen = PropertyBitField::new(); for sub_list in applicable_declarations.iter().rev() { - for declaration in sub_list.declarations.iter().rev() { + for declaration in sub_list.declarations.iter() { match *declaration { % for style_struct in STYLE_STRUCTS: % for property in style_struct.longhands: From e405f81065b220638241654fd6e378a052328911 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 6 May 2014 18:02:13 +0100 Subject: [PATCH 4/8] Set the border-*-width computed values to 0 as appropriate. Fix #2288 --- src/components/main/layout/model.rs | 18 ++++-------------- src/components/style/properties.rs.mako | 10 ++++++++++ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/components/main/layout/model.rs b/src/components/main/layout/model.rs index 14b312a0596..3fda7357fe7 100644 --- a/src/components/main/layout/model.rs +++ b/src/components/main/layout/model.rs @@ -9,7 +9,6 @@ use layout::box_::Box; use computed = style::computed_values; use geom::SideOffsets2D; use style::computed_values::{LPA_Auto, LPA_Length, LPA_Percentage, LP_Length, LP_Percentage}; -use style::computed_values::{border_style}; use style::ComputedValues; use servo_util::geometry::Au; use servo_util::geometry; @@ -316,20 +315,11 @@ pub fn specified(length: computed::LengthOrPercentage, containing_length: Au) -> #[inline] pub fn border_from_style(style: &ComputedValues) -> SideOffsets2D { - #[inline] - fn width(width: Au, style: border_style::T) -> Au { - if style == border_style::none { - Au(0) - } else { - width - } - } - let border_style = style.Border.get(); - SideOffsets2D::new(width(border_style.border_top_width, border_style.border_top_style), - width(border_style.border_right_width, border_style.border_right_style), - width(border_style.border_bottom_width, border_style.border_bottom_style), - width(border_style.border_left_width, border_style.border_left_style)) + SideOffsets2D::new(border_style.border_top_width, + border_style.border_right_width, + border_style.border_bottom_width, + border_style.border_left_width) } #[inline] diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 00e572f6d64..bb2de3571a4 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1878,6 +1878,16 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], } } + { + let border = style_Border.get_mut(); + % for side in ["top", "right", "bottom", "left"]: + // Like calling to_computed_value, which wouldn't type check. + if !(seen.get_border_${side}_width() || context.border_${side}_present) { + border.border_${side}_width = Au(0); + } + % endfor + } + (ComputedValues { % for style_struct in STYLE_STRUCTS: ${style_struct.name}: style_${style_struct.name}, From 4d80cfb4049ad44be89afd1b7ed98c7431bb9295 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 6 May 2014 18:40:01 +0100 Subject: [PATCH 5/8] Fix computed value of 'display' when no declaration applies. --- src/components/style/properties.rs.mako | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index bb2de3571a4..83b70567717 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1888,6 +1888,11 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], % endfor } + if !seen.get_display() { + let box_ = style_Box.get_mut(); + box_.display = longhands::display::to_computed_value(box_.display, &context); + } + (ComputedValues { % for style_struct in STYLE_STRUCTS: ${style_struct.name}: style_${style_struct.name}, From 8186d4d4295e1ffd52716ab526896eb62b62a6f4 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 8 May 2014 15:14:55 +0100 Subject: [PATCH 6/8] Try random whitespace changes to maybe fix an issue that I can not reproduce. --- src/components/style/properties.rs.mako | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 83b70567717..9d49a946776 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1523,9 +1523,10 @@ impl PropertyDeclaration { if !seen.get_${sub_property.ident}() { seen.set_${sub_property.ident}(); result_list.push(${sub_property.ident}_declaration( - CSSWideKeyword(${ - "Inherit" if sub_property.style_struct.inherited else "Initial" - }))); + CSSWideKeyword( + ${"Inherit" if sub_property.style_struct.inherited else "Initial"} + ) + )); } % endfor ValidOrIgnoredDeclaration From 2afe04886353ce61243eca8fb10ed2e082687068 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 8 May 2014 17:19:15 +0100 Subject: [PATCH 7/8] =?UTF-8?q?Fix=20iteration=20order=20for=20building=20?= =?UTF-8?q?computed=20values=E2=80=99=20"context"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/components/style/properties.rs.mako | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 9d49a946776..40cd74a5b48 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1623,7 +1623,10 @@ fn cascade_with_cached_declarations(applicable_declarations: &[MatchedProperty], % endfor let mut seen = PropertyBitField::new(); + // Declaration blocks are stored in increasing precedence order, + // we want them in decreasing order here. for sub_list in applicable_declarations.iter().rev() { + // Declarations are already stored in reverse order. for declaration in sub_list.declarations.iter() { match *declaration { % for style_struct in STYLE_STRUCTS: @@ -1756,8 +1759,10 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], ) // Initialize `context` + // Declarations blocks are already stored in increasing precedence order. for sub_list in applicable_declarations.iter() { - for declaration in sub_list.declarations.iter() { + // Declarations are stored in reverse source order, we want them in forward order here. + for declaration in sub_list.declarations.iter().rev() { match *declaration { font_size_declaration(ref value) => { context.font_size = match *value { @@ -1823,7 +1828,10 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], % endfor let mut cacheable = true; let mut seen = PropertyBitField::new(); + // Declaration blocks are stored in increasing precedence order, + // we want them in decreasing order here. for sub_list in applicable_declarations.iter().rev() { + // Declarations are already stored in reverse order. for declaration in sub_list.declarations.iter() { match *declaration { % for style_struct in STYLE_STRUCTS: From 4a5802bff8c24cd7f3dcdbe2d155d5f3780c0983 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 8 May 2014 18:21:52 +0100 Subject: [PATCH 8/8] This should not make any difference AFAICT, but seems to fix some layout bugs. --- src/components/style/properties.rs.mako | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/style/properties.rs.mako b/src/components/style/properties.rs.mako index 40cd74a5b48..62cce5f1ef4 100644 --- a/src/components/style/properties.rs.mako +++ b/src/components/style/properties.rs.mako @@ -1887,16 +1887,18 @@ pub fn cascade(applicable_declarations: &[MatchedProperty], } } + // The initial value of border-*-width may be changed at computed value time. { let border = style_Border.get_mut(); % for side in ["top", "right", "bottom", "left"]: // Like calling to_computed_value, which wouldn't type check. - if !(seen.get_border_${side}_width() || context.border_${side}_present) { + if !context.border_${side}_present { border.border_${side}_width = Au(0); } % endfor } + // The initial value of display may be changed at computed value time. if !seen.get_display() { let box_ = style_Box.get_mut(); box_.display = longhands::display::to_computed_value(box_.display, &context);