From 0c0018e4d6ba85933a6767f9e8d589cc9517b6d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 3 Sep 2018 22:52:12 +0000 Subject: [PATCH 1/5] style: Make in shadow tree apply the rules from the originating tree. Differential Revision: https://phabricator.services.mozilla.com/D4674 --- components/style/stylist.rs | 53 +++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 038c41ab103..5501b751da2 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1249,7 +1249,8 @@ impl Stylist { } } - if let Some(containing_shadow) = rule_hash_target.containing_shadow() { + let mut current_containing_shadow = rule_hash_target.containing_shadow(); + while let Some(containing_shadow) = current_containing_shadow { let cascade_data = containing_shadow.style_data(); let host = containing_shadow.host(); if let Some(map) = cascade_data.and_then(|data| data.normal_rules(pseudo_element)) { @@ -1267,31 +1268,37 @@ impl Stylist { shadow_cascade_order += 1; } - // NOTE(emilio): Hack so matches document rules as - // expected. - // - // This is not a problem for invalidation and that kind of stuff - // because they still don't match rules based on elements - // outside of the shadow tree, and because the subtree - // is immutable and recreated each time the source tree changes. - // - // See: https://github.com/w3c/svgwg/issues/504 - // - // Note that we always resolve URLs against the document, so we - // can't get into a nested shadow situation here. - // - // See: https://github.com/w3c/svgwg/issues/505 - // - // FIXME(emilio, bug 1487259): We now do after bug 1483882, we - // should jump out of the shadow tree chain now. - // - // Unless the used node is cross-doc, I guess, in which case doc - // rules are probably ok... - let host_is_svg_use = + let host_is_svg_use_element = host.is_svg_element() && host.local_name() == &*local_name!("use"); - match_document_author_rules = host_is_svg_use; + if !host_is_svg_use_element { + match_document_author_rules = false; + break; + } + + debug_assert!( + cascade_data.is_none(), + "We allow no stylesheets in subtrees" + ); + + // NOTE(emilio): Hack so matches the rules of the + // enclosing tree. + // + // This is not a problem for invalidation and that kind of stuff + // because they still don't match rules based on elements + // outside of the shadow tree, and because the + // subtrees are immutable and recreated each time the source + // tree changes. + // + // We historically allow cross-document to have these + // rules applied, but I think that's not great. Gecko is the + // only engine supporting that. + // + // See https://github.com/w3c/svgwg/issues/504 for the relevant + // spec discussion. + current_containing_shadow = host.containing_shadow(); + match_document_author_rules = current_containing_shadow.is_none(); } } From 3f08d2cc0204a86753fcd52736d5a60eeeb2dc33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 3 Sep 2018 07:31:29 +0000 Subject: [PATCH 2/5] style: Make the author styles disabled stuff actually disable style attribute, animations, and XBL rules. This also removes one of my FIXMEs from when I was looking at this code. We don't seem to have a pre-existing test for this feature, sigh. I'll try to write one if I have cycles for it... Note that it not applying XBL rules is a feature, given the current state of affairs. Video controls and such are right now unusable with no styles enabled. Differential Revision: https://phabricator.services.mozilla.com/D4795 --- components/style/stylist.rs | 198 ++++++++++++++++++------------------ 1 file changed, 98 insertions(+), 100 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 5501b751da2..e9cd63e738a 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1129,8 +1129,6 @@ impl Stylist { let rule_hash_target = element.rule_hash_target(); let matches_user_rules = rule_hash_target.matches_user_and_author_rules(); - let matches_author_rules = - matches_user_rules && self.author_styles_enabled == AuthorStylesEnabled::Yes; // Normal user-agent rules. if let Some(map) = self.cascade_data @@ -1195,7 +1193,11 @@ impl Stylist { } } - let mut match_document_author_rules = matches_author_rules; + if self.author_styles_enabled == AuthorStylesEnabled::No { + return; + } + + let mut match_document_author_rules = matches_user_rules; let mut shadow_cascade_order = 0; // XBL / Shadow DOM rules, which are author rules too. @@ -1204,106 +1206,102 @@ impl Stylist { // particular, normally document rules override ::slotted() rules, but // for !important it should be the other way around. So probably we need // to add some sort of AuthorScoped cascade level or something. - if matches_author_rules { - if let Some(shadow) = rule_hash_target.shadow_root() { - if let Some(map) = shadow.style_data().and_then(|data| data.host_rules(pseudo_element)) { - context.with_shadow_host(Some(rule_hash_target), |context| { - map.get_all_matching_rules( - element, - rule_hash_target, - applicable_declarations, - context, - flags_setter, - CascadeLevel::InnerShadowNormal, - shadow_cascade_order, - ); - }); - shadow_cascade_order += 1; - } - } - - // Match slotted rules in reverse order, so that the outer slotted - // rules come before the inner rules (and thus have less priority). - let mut slots = SmallVec::<[_; 3]>::new(); - let mut current = rule_hash_target.assigned_slot(); - while let Some(slot) = current { - slots.push(slot); - current = slot.assigned_slot(); - } - - for slot in slots.iter().rev() { - let shadow = slot.containing_shadow().unwrap(); - if let Some(map) = shadow.style_data().and_then(|data| data.slotted_rules(pseudo_element)) { - context.with_shadow_host(Some(shadow.host()), |context| { - map.get_all_matching_rules( - element, - rule_hash_target, - applicable_declarations, - context, - flags_setter, - CascadeLevel::InnerShadowNormal, - shadow_cascade_order, - ); - }); - shadow_cascade_order += 1; - } - } - - let mut current_containing_shadow = rule_hash_target.containing_shadow(); - while let Some(containing_shadow) = current_containing_shadow { - let cascade_data = containing_shadow.style_data(); - let host = containing_shadow.host(); - if let Some(map) = cascade_data.and_then(|data| data.normal_rules(pseudo_element)) { - context.with_shadow_host(Some(host), |context| { - map.get_all_matching_rules( - element, - rule_hash_target, - applicable_declarations, - context, - flags_setter, - CascadeLevel::SameTreeAuthorNormal, - shadow_cascade_order, - ); - }); - shadow_cascade_order += 1; - } - - let host_is_svg_use_element = - host.is_svg_element() && - host.local_name() == &*local_name!("use"); - - if !host_is_svg_use_element { - match_document_author_rules = false; - break; - } - - debug_assert!( - cascade_data.is_none(), - "We allow no stylesheets in subtrees" - ); - - // NOTE(emilio): Hack so matches the rules of the - // enclosing tree. - // - // This is not a problem for invalidation and that kind of stuff - // because they still don't match rules based on elements - // outside of the shadow tree, and because the - // subtrees are immutable and recreated each time the source - // tree changes. - // - // We historically allow cross-document to have these - // rules applied, but I think that's not great. Gecko is the - // only engine supporting that. - // - // See https://github.com/w3c/svgwg/issues/504 for the relevant - // spec discussion. - current_containing_shadow = host.containing_shadow(); - match_document_author_rules = current_containing_shadow.is_none(); + if let Some(shadow) = rule_hash_target.shadow_root() { + if let Some(map) = shadow.style_data().and_then(|data| data.host_rules(pseudo_element)) { + context.with_shadow_host(Some(rule_hash_target), |context| { + map.get_all_matching_rules( + element, + rule_hash_target, + applicable_declarations, + context, + flags_setter, + CascadeLevel::InnerShadowNormal, + shadow_cascade_order, + ); + }); + shadow_cascade_order += 1; } } - // FIXME(emilio): This doesn't account for the author_styles_enabled - // stuff... + // Match slotted rules in reverse order, so that the outer slotted + // rules come before the inner rules (and thus have less priority). + let mut slots = SmallVec::<[_; 3]>::new(); + let mut current = rule_hash_target.assigned_slot(); + while let Some(slot) = current { + slots.push(slot); + current = slot.assigned_slot(); + } + + for slot in slots.iter().rev() { + let shadow = slot.containing_shadow().unwrap(); + if let Some(map) = shadow.style_data().and_then(|data| data.slotted_rules(pseudo_element)) { + context.with_shadow_host(Some(shadow.host()), |context| { + map.get_all_matching_rules( + element, + rule_hash_target, + applicable_declarations, + context, + flags_setter, + CascadeLevel::InnerShadowNormal, + shadow_cascade_order, + ); + }); + shadow_cascade_order += 1; + } + } + + let mut current_containing_shadow = rule_hash_target.containing_shadow(); + while let Some(containing_shadow) = current_containing_shadow { + let cascade_data = containing_shadow.style_data(); + let host = containing_shadow.host(); + if let Some(map) = cascade_data.and_then(|data| data.normal_rules(pseudo_element)) { + context.with_shadow_host(Some(host), |context| { + map.get_all_matching_rules( + element, + rule_hash_target, + applicable_declarations, + context, + flags_setter, + CascadeLevel::SameTreeAuthorNormal, + shadow_cascade_order, + ); + }); + shadow_cascade_order += 1; + } + + let host_is_svg_use_element = + host.is_svg_element() && + host.local_name() == &*local_name!("use"); + + if !host_is_svg_use_element { + match_document_author_rules = false; + break; + } + + debug_assert!( + cascade_data.is_none(), + "We allow no stylesheets in subtrees" + ); + + // NOTE(emilio): Hack so matches the rules of the + // enclosing tree. + // + // This is not a problem for invalidation and that kind of stuff + // because they still don't match rules based on elements + // outside of the shadow tree, and because the + // subtrees are immutable and recreated each time the source + // tree changes. + // + // We historically allow cross-document to have these + // rules applied, but I think that's not great. Gecko is the + // only engine supporting that. + // + // See https://github.com/w3c/svgwg/issues/504 for the relevant + // spec discussion. + current_containing_shadow = host.containing_shadow(); + match_document_author_rules = current_containing_shadow.is_none(); + } + let cut_xbl_binding_inheritance = element.each_xbl_cascade_data(|cascade_data, quirks_mode| { if let Some(map) = cascade_data.normal_rules(pseudo_element) { From cf6215c85f66390bbcddd05f80d37590a9920d3d Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 5 Sep 2018 04:50:33 +0000 Subject: [PATCH 3/5] style: Add a pref for |clip-path:path()|. Add a preference, layout.css.clip-path-path.enabled, for |clip-path:path()|. Differential Revision: https://phabricator.services.mozilla.com/D4965 --- .../style/values/specified/basic_shape.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs index 824c13f1f01..18541b8dbce 100644 --- a/components/style/values/specified/basic_shape.rs +++ b/components/style/values/specified/basic_shape.rs @@ -49,17 +49,24 @@ pub type ShapeRadius = generic::ShapeRadius; /// The specified value of `Polygon` pub type Polygon = generic::Polygon; +#[cfg(feature = "gecko")] +fn is_clip_path_path_enabled(context: &ParserContext) -> bool { + use gecko_bindings::structs::mozilla; + context.chrome_rules_enabled() || + unsafe { mozilla::StaticPrefs_sVarCache_layout_css_clip_path_path_enabled } +} +#[cfg(feature = "servo")] +fn is_clip_path_path_enabled(_: &ParserContext) -> bool { + false +} + impl Parse for ClippingShape { #[inline] fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - // |clip-path:path()| is a chrome-only property value support for now. `path()` is - // defined in css-shape-2, but the spec is not stable enough, and we haven't decided - // to make it public yet. However, it has some benefits for the front-end, so we - // implement it. - if context.chrome_rules_enabled() { + if is_clip_path_path_enabled(context) { if let Ok(p) = input.try(|i| Path::parse(context, i)) { return Ok(ShapeSource::Path(p)); } From 3e0250ae61fcac63361e0b879aabafb13feefbf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 5 Sep 2018 12:52:16 +0000 Subject: [PATCH 4/5] style: Only no-op visited <-> unvisited changes. Other changes should really be (and are) indistinguishable. Differential Revision: https://phabricator.services.mozilla.com/D4847 --- .../element/state_and_attributes.rs | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index fa0ccdd8be9..d13da58a89b 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -155,22 +155,11 @@ where return false; } - // If we are sensitive to visitedness and the visited state changed, we - // force a restyle here. Matching doesn't depend on the actual visited - // state at all, so we can't look at matching results to decide what to - // do for this case. - if state_changes.intersects(ElementState::IN_VISITED_OR_UNVISITED_STATE) && - self.shared_context.visited_styles_enabled - { + // If we the visited state changed, we force a restyle here. Matching + // doesn't depend on the actual visited state at all, so we can't look + // at matching results to decide what to do for this case. + if state_changes.intersects(ElementState::IN_VISITED_OR_UNVISITED_STATE) { trace!(" > visitedness change, force subtree restyle"); - // We shouldn't get here with visited links disabled, but it's hard - // to assert in cases where you record a visitedness change and - // afterwards you change some of the stuff (like the pref) that - // changes whether visited styles are enabled. - // - // So just avoid the restyle here, because it kind of would kill the - // point of disabling visited links. - // // We can't just return here because there may also be attribute // changes as well that imply additional hints for siblings. self.data.hint.insert(RestyleHint::restyle_subtree()); From 6dcf9b763060d126d520843cb877c49488728edb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 5 Sep 2018 13:35:06 +0000 Subject: [PATCH 5/5] style: Use decomposition to interpolate matched perspective transform operations. Looks like this produces sensible results for interpolation with 0, though I'm not really convinced about the results from, let's say, 1px to 2000px in the attached test-case, I would've expected a linear interpolation from that to go through normal length interpolation. css-transforms-1 says: > Two transform functions with the same name and the same number of arguments > are interpolated numerically without a former conversion. The calculated > value will be of the same transform function type with the same number of > arguments. > > Special rules apply to . Which is what we do... I was going to file a spec issue but turns out that it's already addressed in css-transforms-2: https://drafts.csswg.org/css-transforms-2/#interpolation-of-transform-functions Which says: > The transform functions , matrix3d() and perspective() get > converted into 4x4 matrices first and interpolated as defined in section > Interpolation of Matrices afterwards. Differential Revision: https://phabricator.services.mozilla.com/D4942 --- .../helpers/animated_properties.mako.rs | 24 ++++++++++++++++--- components/style/values/computed/transform.rs | 1 + 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index f6277a21271..437232d386a 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1296,9 +1296,27 @@ impl Animate for ComputedTransformOperation { &TransformOperation::Perspective(ref fd), &TransformOperation::Perspective(ref td), ) => { - Ok(TransformOperation::Perspective( - fd.animate(td, procedure)? - )) + use values::computed::CSSPixelLength; + use values::generics::transform::create_perspective_matrix; + + // From https://drafts.csswg.org/css-transforms-2/#interpolation-of-transform-functions: + // + // The transform functions matrix(), matrix3d() and + // perspective() get converted into 4x4 matrices first and + // interpolated as defined in section Interpolation of + // Matrices afterwards. + // + let from = create_perspective_matrix(fd.px()); + let to = create_perspective_matrix(td.px()); + + let interpolated = + Matrix3D::from(from).animate(&Matrix3D::from(to), procedure)?; + + let decomposed = decompose_3d_matrix(interpolated)?; + let perspective_z = decomposed.perspective.2; + let used_value = + if perspective_z == 0. { 0. } else { -1. / perspective_z }; + Ok(TransformOperation::Perspective(CSSPixelLength::new(used_value))) }, _ if self.is_translate() && other.is_translate() => { self.to_translate_3d().animate(&other.to_translate_3d(), procedure) diff --git a/components/style/values/computed/transform.rs b/components/style/values/computed/transform.rs index bbf710c4295..a2849a0d4b3 100644 --- a/components/style/values/computed/transform.rs +++ b/components/style/values/computed/transform.rs @@ -42,6 +42,7 @@ impl TransformOrigin { /// computed value of matrix3d() pub type Matrix3D = generic::Matrix3D; + /// computed value of matrix() pub type Matrix = generic::Matrix;