Skip to content

Commit

Permalink
Auto merge of #17875 - bzbarsky:first-line-dual-inheritance, r=emilio
Browse files Browse the repository at this point in the history 
Add support for having two separate parent styles.  Fixes gecko bug 1382806.

<!-- Please describe your changes on the following line: -->

This is needed for ::first-line support.  See https://drafts.csswg.org/css-pseudo-4/#first-line-inheritance

This PR doesn't quite implement what the CSS spec draft says right now.  It implements what Gecko does, which is what an earlier draft said.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix https://bugzilla.mozilla.org/show_bug.cgi?id=1382806

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because servo doesn't support ::first-line yet

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17875)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Jul 26, 2017
2 parents a15d13a + 048044f commit 020188f
Show file tree
Hide file tree
Showing 11 changed files with 109 additions and 33 deletions.
1 change: 1 addition & 0 deletions components/style/animation.rs
View file
Expand Up @@ -499,6 +499,7 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
iter,
Some(previous_style),
Some(previous_style),
Some(previous_style),
/* cascade_info = */ None,
/* visited_style = */ None,
font_metrics_provider,
Expand Down
4 changes: 2 additions & 2 deletions components/style/properties/helpers/animated_properties.mako.rs
View file
Expand Up @@ -605,8 +605,8 @@ impl AnimationValue {
CSSWideKeyword::Unset |
% endif
CSSWideKeyword::Inherit => {
let inherit_struct = context.inherited_style()
.get_${prop.style_struct.name_lower}();
let inherit_struct = context.builder
.get_parent_${prop.style_struct.name_lower}();
inherit_struct.clone_${prop.ident}()
},
};
Expand Down
2 changes: 1 addition & 1 deletion components/style/properties/longhand/color.mako.rs
View file
Expand Up @@ -22,7 +22,7 @@
#[inline]
fn to_computed_value(&self, context: &Context) -> computed_value::T {
self.0.to_computed_value(context)
.to_rgba(context.inherited_style().get_color().clone_color())
.to_rgba(context.builder.get_parent_color().clone_color())
}

#[inline]
Expand Down
24 changes: 11 additions & 13 deletions components/style/properties/longhand/font.mako.rs
View file
Expand Up @@ -548,9 +548,9 @@ ${helpers.single_keyword_system("font-variant-caps",
SpecifiedValue::Normal => computed_value::T::normal(),
SpecifiedValue::Bold => computed_value::T::bold(),
SpecifiedValue::Bolder =>
context.inherited_style().get_font().clone_font_weight().bolder(),
context.builder.get_parent_font().clone_font_weight().bolder(),
SpecifiedValue::Lighter =>
context.inherited_style().get_font().clone_font_weight().lighter(),
context.builder.get_parent_font().clone_font_weight().lighter(),
SpecifiedValue::System(_) => {
<%self:nongecko_unreachable>
context.cached_system_font.as_ref().unwrap().font_weight.clone()
Expand Down Expand Up @@ -940,7 +940,7 @@ ${helpers.single_keyword_system("font-variant-caps",
// recomputed from the base size for the keyword and the relative size.
//
// See bug 1355707
if let Some((kw, fraction)) = context.builder.inherited_style().font_computation_data.font_size_keyword {
if let Some((kw, fraction)) = context.builder.inherited_font_computation_data().font_size_keyword {
context.builder.font_size_keyword = Some((kw, fraction * ratio));
} else {
context.builder.font_size_keyword = None;
Expand All @@ -956,9 +956,9 @@ ${helpers.single_keyword_system("font-variant-caps",
// if the language or generic changed, we need to recalculate
// the font size from the stored font-size origin information.
if context.builder.get_font().gecko().mLanguage.raw::<nsIAtom>() !=
context.builder.inherited_style().get_font().gecko().mLanguage.raw::<nsIAtom>() ||
context.builder.get_parent_font().gecko().mLanguage.raw::<nsIAtom>() ||
context.builder.get_font().gecko().mGenericID !=
context.builder.inherited_style().get_font().gecko().mGenericID {
context.builder.get_parent_font().gecko().mGenericID {
if let Some((kw, ratio)) = context.builder.font_size_keyword {
computed = kw.to_computed_value(context).scale_by(ratio);
}
Expand All @@ -968,8 +968,7 @@ ${helpers.single_keyword_system("font-variant-caps",
let device = context.builder.device;
let mut font = context.builder.take_font();
let parent_unconstrained = {
let parent_style = context.builder.inherited_style();
let parent_font = parent_style.get_font();
let parent_font = context.builder.get_parent_font();
font.apply_font_size(computed, parent_font, device)
};
context.builder.put_font(font);
Expand Down Expand Up @@ -997,9 +996,8 @@ ${helpers.single_keyword_system("font-variant-caps",
let device = context.builder.device;
let mut font = context.builder.take_font();
let used_kw = {
let parent_style = context.builder.inherited_style();
let parent_font = parent_style.get_font();
parent_kw = parent_style.font_computation_data.font_size_keyword;
let parent_font = context.builder.get_parent_font();
parent_kw = context.builder.inherited_font_computation_data().font_size_keyword;

font.inherit_font_size_from(parent_font, kw_inherited_size, device)
};
Expand Down Expand Up @@ -2279,16 +2277,16 @@ https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-control-

let int = match *self {
SpecifiedValue::Auto => {
let parent = cx.inherited_style().get_font().clone__moz_script_level() as i32;
let display = cx.inherited_style().get_font().clone__moz_math_display();
let parent = cx.builder.get_parent_font().clone__moz_script_level() as i32;
let display = cx.builder.get_parent_font().clone__moz_math_display();
if display == DisplayValue::inline {
parent + 1
} else {
parent
}
}
SpecifiedValue::Relative(rel) => {
let parent = cx.inherited_style().get_font().clone__moz_script_level();
let parent = cx.builder.get_parent_font().clone__moz_script_level();
parent as i32 + rel
}
SpecifiedValue::Absolute(abs) => abs,
Expand Down
8 changes: 4 additions & 4 deletions components/style/properties/longhand/inherited_text.mako.rs
View file
Expand Up @@ -230,8 +230,8 @@ ${helpers.single_keyword("text-align-last",
if context.is_root_element {
return get_initial_value();
}
let parent = context.inherited_style().get_inheritedtext().clone_text_align();
let ltr = context.inherited_style().writing_mode.is_bidi_ltr();
let parent = context.builder.get_parent_inheritedtext().clone_text_align();
let ltr = context.builder.inherited_writing_mode().is_bidi_ltr();
match (parent, ltr) {
(computed_value::T::start, true) => computed_value::T::left,
(computed_value::T::start, false) => computed_value::T::right,
Expand All @@ -241,7 +241,7 @@ ${helpers.single_keyword("text-align-last",
}
}
SpecifiedValue::MozCenterOrInherit => {
let parent = context.inherited_style().get_inheritedtext().clone_text_align();
let parent = context.builder.get_parent_inheritedtext().clone_text_align();
if parent == computed_value::T::start {
computed_value::T::center
} else {
Expand Down Expand Up @@ -340,7 +340,7 @@ ${helpers.predefined_type("word-spacing",
overline: None,
line_through: None,
},
_ => context.inherited_style().get_inheritedtext().clone__servo_text_decorations_in_effect()
_ => context.builder.get_parent_inheritedtext().clone__servo_text_decorations_in_effect()
};

result.underline = maybe(context.style().get_text().has_underline()
Expand Down
87 changes: 82 additions & 5 deletions components/style/properties/properties.mako.rs
View file
Expand Up @@ -14,6 +14,7 @@ use servo_arc::{Arc, UniqueArc};
use std::borrow::Cow;
use std::collections::HashSet;
use std::{fmt, mem, ops};
#[cfg(feature = "gecko")] use std::ptr;

use app_units::Au;
#[cfg(feature = "servo")] use cssparser::RGBA;
Expand Down Expand Up @@ -2470,6 +2471,11 @@ pub struct StyleBuilder<'a> {
/// `parent_style.unwrap_or(device.default_computed_values())`.
inherited_style: &'a ComputedValues,

/// The style we're inheriting from for properties that don't inherit from
/// ::first-line. This is the same as inherited_style, unless
/// inherited_style is a ::first-line style.
inherited_style_ignoring_first_line: &'a ComputedValues,

/// The style we're getting reset structs from.
reset_style: &'a ComputedValues,

Expand Down Expand Up @@ -2507,6 +2513,7 @@ impl<'a> StyleBuilder<'a> {
fn new(
device: &'a Device,
parent_style: Option<<&'a ComputedValues>,
parent_style_ignoring_first_line: Option<<&'a ComputedValues>,
pseudo: Option<<&'a PseudoElement>,
cascade_flags: CascadeFlags,
rules: Option<StrongRuleNode>,
Expand All @@ -2516,8 +2523,19 @@ impl<'a> StyleBuilder<'a> {
flags: ComputedValueFlags,
visited_style: Option<Arc<ComputedValues>>,
) -> Self {
debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
#[cfg(feature = "gecko")]
debug_assert!(parent_style.is_none() ||
ptr::eq(parent_style.unwrap(),
parent_style_ignoring_first_line.unwrap()) ||
parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine));
let reset_style = device.default_computed_values();
let inherited_style = parent_style.unwrap_or(reset_style);
let inherited_style_ignoring_first_line = parent_style_ignoring_first_line.unwrap_or(reset_style);
// FIXME(bz): INHERIT_ALL seems like a fundamentally broken idea. I'm
// 99% sure it should give incorrect behavior for table anonymous box
// backgrounds, for example. This code doesn't attempt to make it play
// nice with inherited_style_ignoring_first_line.
let reset_style = if cascade_flags.contains(INHERIT_ALL) {
inherited_style
} else {
Expand All @@ -2528,6 +2546,7 @@ impl<'a> StyleBuilder<'a> {
device,
parent_style,
inherited_style,
inherited_style_ignoring_first_line,
reset_style,
pseudo,
rules,
Expand Down Expand Up @@ -2556,10 +2575,15 @@ impl<'a> StyleBuilder<'a> {
) -> Self {
let reset_style = device.default_computed_values();
let inherited_style = parent_style.unwrap_or(reset_style);
#[cfg(feature = "gecko")]
debug_assert!(parent_style.is_none() ||
parent_style.unwrap().pseudo() != Some(PseudoElement::FirstLine));
StyleBuilder {
device,
parent_style,
inherited_style,
// None of our callers pass in ::first-line parent styles.
inherited_style_ignoring_first_line: inherited_style,
reset_style,
pseudo,
rules: None, // FIXME(emilio): Dubious...
Expand All @@ -2581,8 +2605,13 @@ impl<'a> StyleBuilder<'a> {
/// Inherit `${property.ident}` from our parent style.
#[allow(non_snake_case)]
pub fn inherit_${property.ident}(&mut self) {
% if property.style_struct.inherited:
let inherited_struct =
self.inherited_style.get_${property.style_struct.name_lower}();
% else:
let inherited_struct =
self.inherited_style_ignoring_first_line.get_${property.style_struct.name_lower}();
% endif
self.${property.style_struct.ident}.mutate()
.copy_${property.ident}_from(
inherited_struct,
Expand Down Expand Up @@ -2639,6 +2668,7 @@ impl<'a> StyleBuilder<'a> {
Self::new(
device,
Some(parent),
Some(parent),
pseudo,
CascadeFlags::empty(),
/* rules = */ None,
Expand All @@ -2655,11 +2685,6 @@ impl<'a> StyleBuilder<'a> {
self.visited_style.is_some()
}

/// Returns the style we're inheriting from.
pub fn inherited_style(&self) -> &'a ComputedValues {
self.inherited_style
}

/// Returns the style we're getting reset properties from.
pub fn default_style(&self) -> &'a ComputedValues {
self.reset_style
Expand Down Expand Up @@ -2752,6 +2777,42 @@ impl<'a> StyleBuilder<'a> {
fn custom_properties(&self) -> Option<Arc<::custom_properties::CustomPropertiesMap>> {
self.custom_properties.clone()
}

/// Access to various information about our inherited styles. We don't
/// expose an inherited ComputedValues directly, because in the
/// ::first-line case some of the inherited information needs to come from
/// one ComputedValues instance and some from a different one.

/// Inherited font bits.
pub fn inherited_font_computation_data(&self) -> &FontComputationData {
&self.inherited_style.font_computation_data
}

/// Inherited writing-mode.
pub fn inherited_writing_mode(&self) -> &WritingMode {
&self.inherited_style.writing_mode
}

/// Inherited style flags.
pub fn inherited_flags(&self) -> &ComputedValueFlags {
&self.inherited_style.flags
}

/// And access to inherited style structs.
% for style_struct in data.active_style_structs():
/// Gets our inherited `${style_struct.name}`. We don't name these
/// accessors `inherited_${style_struct.name_lower}` because we already
/// have things like "box" vs "inherited_box" as struct names. Do the
/// next-best thing and call them `parent_${style_struct.name_lower}`
/// instead.
pub fn get_parent_${style_struct.name_lower}(&self) -> &style_structs::${style_struct.name} {
% if style_struct.inherited:
self.inherited_style.get_${style_struct.name_lower}()
% else:
self.inherited_style_ignoring_first_line.get_${style_struct.name_lower}()
% endif
}
% endfor
}

#[cfg(feature = "servo")]
Expand Down Expand Up @@ -2867,13 +2928,20 @@ pub fn cascade(
rule_node: &StrongRuleNode,
guards: &StylesheetGuards,
parent_style: Option<<&ComputedValues>,
parent_style_ignoring_first_line: Option<<&ComputedValues>,
layout_parent_style: Option<<&ComputedValues>,
visited_style: Option<Arc<ComputedValues>>,
cascade_info: Option<<&mut CascadeInfo>,
font_metrics_provider: &FontMetricsProvider,
flags: CascadeFlags,
quirks_mode: QuirksMode
) -> Arc<ComputedValues> {
debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
#[cfg(feature = "gecko")]
debug_assert!(parent_style.is_none() ||
ptr::eq(parent_style.unwrap(),
parent_style_ignoring_first_line.unwrap()) ||
parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine));
let iter_declarations = || {
rule_node.self_and_ancestors().flat_map(|node| {
let cascade_level = node.cascade_level();
Expand Down Expand Up @@ -2919,6 +2987,7 @@ pub fn cascade(
rule_node,
iter_declarations,
parent_style,
parent_style_ignoring_first_line,
layout_parent_style,
visited_style,
cascade_info,
Expand All @@ -2937,6 +3006,7 @@ pub fn apply_declarations<'a, F, I>(
rules: &StrongRuleNode,
iter_declarations: F,
parent_style: Option<<&ComputedValues>,
parent_style_ignoring_first_line: Option<<&ComputedValues>,
layout_parent_style: Option<<&ComputedValues>,
visited_style: Option<Arc<ComputedValues>>,
mut cascade_info: Option<<&mut CascadeInfo>,
Expand All @@ -2949,6 +3019,12 @@ where
I: Iterator<Item = (&'a PropertyDeclaration, CascadeLevel)>,
{
debug_assert!(layout_parent_style.is_none() || parent_style.is_some());
debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
#[cfg(feature = "gecko")]
debug_assert!(parent_style.is_none() ||
ptr::eq(parent_style.unwrap(),
parent_style_ignoring_first_line.unwrap()) ||
parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine));
let (inherited_style, layout_parent_style) = match parent_style {
Some(parent_style) => {
(parent_style,
Expand Down Expand Up @@ -2983,6 +3059,7 @@ where
builder: StyleBuilder::new(
device,
parent_style,
parent_style_ignoring_first_line,
pseudo,
flags,
Some(rules.clone()),
Expand Down
2 changes: 1 addition & 1 deletion components/style/style_adjuster.rs
View file
Expand Up @@ -446,7 +446,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
let relevant_link_visited = if flags.contains(IS_LINK) {
flags.contains(IS_VISITED_LINK)
} else {
self.style.inherited_style().flags.contains(IS_RELEVANT_LINK_VISITED)
self.style.inherited_flags().contains(IS_RELEVANT_LINK_VISITED)
};

if relevant_link_visited {
Expand Down
1 change: 1 addition & 0 deletions components/style/style_resolver.rs
View file
Expand Up @@ -479,6 +479,7 @@ where
rules.unwrap_or(self.context.shared.stylist.rule_tree().root()),
&self.context.shared.guards,
parent_style,
parent_style,
layout_parent_style,
style_if_visited,
Some(&mut cascade_info),
Expand Down
4 changes: 4 additions & 0 deletions components/style/stylist.rs
View file
Expand Up @@ -637,6 +637,7 @@ impl Stylist {
guards,
parent,
parent,
parent,
None,
None,
font_metrics,
Expand Down Expand Up @@ -753,6 +754,7 @@ impl Stylist {
guards,
Some(inherited_style),
Some(inherited_style),
Some(inherited_style),
None,
None,
font_metrics,
Expand All @@ -778,6 +780,7 @@ impl Stylist {
guards,
Some(parent_style),
Some(parent_style),
Some(parent_style),
visited_values,
None,
font_metrics,
Expand Down Expand Up @@ -1342,6 +1345,7 @@ impl Stylist {
guards,
Some(parent_style),
Some(parent_style),
Some(parent_style),
None,
None,
&metrics,
Expand Down

0 comments on commit 020188f

Please sign in to comment.