From 14fb147b6cd96eca615630f20737916b7a57d1c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Aug 2023 00:27:39 +0200 Subject: [PATCH] style: Fix visited handling after bug 1763750 Before bug 1763750, we unconditionally called compute_writing_mode, which got the writing mode from the cascade mode for visited styles. However after that bug we only do that if we apply any writing-mode-related property. We could just call compute_writing_mode unconditionally, but instead it seems better to skip all that work for visited cascade and reuse the mechanism introduced in that bug to only apply the visited-dependent longhands. We assert that all visited-dependent longhands are "late" longhands, so as to also avoid applying the font group and such. Differential Revision: https://phabricator.services.mozilla.com/D143490 --- components/style/properties/cascade.rs | 138 +++++++++--------- .../style/properties/properties.mako.rs | 60 ++++---- 2 files changed, 100 insertions(+), 98 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index fb615375b19..e7d605dfc19 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -296,56 +296,69 @@ where rule_cache_conditions: RefCell::new(rule_cache_conditions), }; - let using_cached_reset_properties = { - let mut cascade = Cascade::new(&mut context, cascade_mode, &referenced_properties); - let mut shorthand_cache = ShorthandsWithPropertyReferencesCache::default(); - if cascade.apply_properties( - CanHaveLogicalProperties::No, - LonghandIdSet::writing_mode_group(), - declarations.iter().cloned(), - &mut shorthand_cache, - ) { - cascade.compute_writing_mode(); + let using_cached_reset_properties; + let mut cascade = Cascade::new(&mut context, cascade_mode, &referenced_properties); + let mut shorthand_cache = ShorthandsWithPropertyReferencesCache::default(); + + let properties_to_apply = match cascade.cascade_mode { + CascadeMode::Visited { writing_mode } => { + cascade.context.builder.writing_mode = writing_mode; + // We never insert visited styles into the cache so we don't need to + // try looking it up. It also wouldn't be super-profitable, only a + // handful reset properties are non-inherited. + using_cached_reset_properties = false; + LonghandIdSet::visited_dependent() + }, + CascadeMode::Unvisited { visited_rules } => { + if cascade.apply_properties( + CanHaveLogicalProperties::No, + LonghandIdSet::writing_mode_group(), + declarations.iter().cloned(), + &mut shorthand_cache, + ) { + cascade.compute_writing_mode(); + } + + if cascade.apply_properties( + CanHaveLogicalProperties::No, + LonghandIdSet::fonts_and_color_group(), + declarations.iter().cloned(), + &mut shorthand_cache, + ) { + cascade.fixup_font_stuff(); + } + + if let Some(visited_rules) = visited_rules { + cascade.compute_visited_style_if_needed( + element, + parent_style, + parent_style_ignoring_first_line, + layout_parent_style, + visited_rules, + guards, + ); + } + + using_cached_reset_properties = + cascade.try_to_use_cached_reset_properties(rule_cache, guards); + + if using_cached_reset_properties { + LonghandIdSet::late_group_only_inherited() + } else { + LonghandIdSet::late_group() + } } - - if cascade.apply_properties( - CanHaveLogicalProperties::No, - LonghandIdSet::fonts_and_color_group(), - declarations.iter().cloned(), - &mut shorthand_cache, - ) { - cascade.fixup_font_stuff(); - } - - cascade.compute_visited_style_if_needed( - element, - parent_style, - parent_style_ignoring_first_line, - layout_parent_style, - guards, - ); - - let using_cached_reset_properties = - cascade.try_to_use_cached_reset_properties(rule_cache, guards); - - let properties_to_apply = if using_cached_reset_properties { - LonghandIdSet::late_group_only_inherited() - } else { - LonghandIdSet::late_group() - }; - - cascade.apply_properties( - CanHaveLogicalProperties::Yes, - properties_to_apply, - declarations.iter().cloned(), - &mut shorthand_cache, - ); - - cascade.finished_applying_properties(); - - using_cached_reset_properties }; + cascade.apply_properties( + CanHaveLogicalProperties::Yes, + properties_to_apply, + declarations.iter().cloned(), + &mut shorthand_cache, + ); + + cascade.finished_applying_properties(); + context.builder.clear_modified_reset(); if matches!(cascade_mode, CascadeMode::Unvisited { .. }) { @@ -627,15 +640,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } } - // Only a few properties are allowed to depend on the visited state - // of links. When cascading visited styles, we can save time by - // only processing these properties. - if matches!(self.cascade_mode, CascadeMode::Visited { .. }) && - !physical_longhand_id.is_visited_dependent() - { - continue; - } - let mut declaration = self.substitute_variables_if_needed(declaration, &mut shorthand_cache); @@ -708,13 +712,9 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } fn compute_writing_mode(&mut self) { - let writing_mode = match self.cascade_mode { - CascadeMode::Unvisited { .. } => { - WritingMode::new(self.context.builder.get_inherited_box()) - }, - CascadeMode::Visited { writing_mode } => writing_mode, - }; - self.context.builder.writing_mode = writing_mode; + debug_assert!(matches!(self.cascade_mode, CascadeMode::Unvisited { .. })); + self.context.builder.writing_mode = + WritingMode::new(self.context.builder.get_inherited_box()) } fn compute_visited_style_if_needed( @@ -723,20 +723,12 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { parent_style: Option<&ComputedValues>, parent_style_ignoring_first_line: Option<&ComputedValues>, layout_parent_style: Option<&ComputedValues>, + visited_rules: &StrongRuleNode, guards: &StylesheetGuards, ) where E: TElement, { - let visited_rules = match self.cascade_mode { - CascadeMode::Unvisited { visited_rules } => visited_rules, - CascadeMode::Visited { .. } => return, - }; - - let visited_rules = match visited_rules { - Some(rules) => rules, - None => return, - }; - + debug_assert!(matches!(self.cascade_mode, CascadeMode::Unvisited { .. })); let is_link = self.context.builder.pseudo.is_none() && element.unwrap().is_link(); macro_rules! visited_parent { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 7b6f8380d2d..bfe4def58d2 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -934,6 +934,29 @@ CASCADE_GROUPS = { def in_late_group(p): return p.name not in CASCADE_GROUPS["writing_mode"] and p.name not in CASCADE_GROUPS["fonts_and_color"] +def is_visited_dependent(p): + return p.name in [ + "column-rule-color", + "text-emphasis-color", + "-webkit-text-fill-color", + "-webkit-text-stroke-color", + "text-decoration-color", + "fill", + "stroke", + "caret-color", + "background-color", + "border-top-color", + "border-right-color", + "border-bottom-color", + "border-left-color", + "border-block-start-color", + "border-inline-end-color", + "border-block-end-color", + "border-inline-start-color", + "outline-color", + "color", + ] + %> impl LonghandIdSet { @@ -972,6 +995,18 @@ impl LonghandIdSet { &IGNORED_WHEN_COLORS_DISABLED } + /// Only a few properties are allowed to depend on the visited state of + /// links. When cascading visited styles, we can save time by only + /// processing these properties. + fn visited_dependent() -> &'static Self { + ${static_longhand_id_set( + "VISITED_DEPENDENT", + lambda p: is_visited_dependent(p) + )} + debug_assert!(Self::late_group().contains_all(&VISITED_DEPENDENT)); + &VISITED_DEPENDENT + } + #[inline] fn writing_mode_group() -> &'static Self { ${static_longhand_id_set( @@ -1404,31 +1439,6 @@ impl LonghandId { PropertyFlags::from_bits_truncate(FLAGS[self as usize]) } - /// Only a few properties are allowed to depend on the visited state of - /// links. When cascading visited styles, we can save time by only - /// processing these properties. - fn is_visited_dependent(&self) -> bool { - matches!(*self, - % if engine == "gecko": - LonghandId::ColumnRuleColor | - LonghandId::TextEmphasisColor | - LonghandId::WebkitTextFillColor | - LonghandId::WebkitTextStrokeColor | - LonghandId::TextDecorationColor | - LonghandId::Fill | - LonghandId::Stroke | - LonghandId::CaretColor | - % endif - LonghandId::BackgroundColor | - LonghandId::BorderTopColor | - LonghandId::BorderRightColor | - LonghandId::BorderBottomColor | - LonghandId::BorderLeftColor | - LonghandId::OutlineColor | - LonghandId::Color - ) - } - /// Returns true if the property is one that is ignored when document /// colors are disabled. #[inline]