Skip to content

Commit 3b02d12

Browse files
jfkthamejfkthame
committed
Bug 2024601 - patch 5 - Provide an AttributeTracker when resolving style query expression values. r=firefox-style-system-reviewers,emilio,dshin
Differential Revision: https://phabricator.services.mozilla.com/D290906
1 parent 6e0fdc9 commit 3b02d12

8 files changed

Lines changed: 137 additions & 55 deletions

File tree

‎servo/components/style/custom_properties.rs‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2582,10 +2582,20 @@ fn substitute_one_reference<'a>(
25822582
},
25832583
|attr| {
25842584
let attr = if let AttributeType::Type(_) = &reference.attribute_data.kind {
2585-
substitution_functions
2586-
.get_attr(&reference.name)
2587-
.map(|v| v.to_variable_value())?
2588-
.css
2585+
// If we're evaluating a container query, we haven't run the cascade
2586+
// and populated substitution_functions.attributes, so we can't do the
2587+
// get_attr() lookup here.
2588+
// TODO: This means chained attr() references will not work reliably in
2589+
// container style queries:
2590+
// https://bugzilla.mozilla.org/show_bug.cgi?id=2028861
2591+
if computed_context.in_container_query {
2592+
attr
2593+
} else {
2594+
substitution_functions
2595+
.get_attr(&reference.name)
2596+
.map(|v| v.to_variable_value())?
2597+
.css
2598+
}
25892599
} else {
25902600
attr
25912601
};

‎servo/components/style/media_queries/media_list.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use super::{MediaQuery, Qualifier};
1010
use crate::context::QuirksMode;
1111
use crate::derives::*;
1212
use crate::device::Device;
13+
use crate::dom::AttributeTracker;
1314
use crate::error_reporting::ContextualParseError;
1415
use crate::parser::ParserContext;
1516
use crate::stylesheets::CustomMediaEvaluator;
@@ -100,9 +101,9 @@ impl MediaList {
100101
}
101102
KleeneValue::any(self.media_queries.iter(), |mq| {
102103
let mut query_match = if mq.media_type.matches(context.device().media_type()) {
103-
mq.condition
104-
.as_ref()
105-
.map_or(KleeneValue::True, |c| c.matches(context, custom))
104+
mq.condition.as_ref().map_or(KleeneValue::True, |c| {
105+
c.matches(context, custom, &mut AttributeTracker::new_dummy())
106+
})
106107
} else {
107108
KleeneValue::False
108109
};

‎servo/components/style/queries/condition.rs‎

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -245,18 +245,26 @@ impl StyleQuery {
245245
}
246246
}
247247

248-
fn matches(&self, ctx: &computed::Context) -> KleeneValue {
248+
fn matches(
249+
&self,
250+
ctx: &computed::Context,
251+
attribute_tracker: &mut AttributeTracker,
252+
) -> KleeneValue {
249253
ctx.builder
250254
.add_flags(ComputedValueFlags::DEPENDS_ON_CONTAINER_STYLE_QUERY);
251255
match *self {
252-
StyleQuery::Feature(ref f) => f.matches(ctx),
253-
StyleQuery::Not(ref c) => !c.matches(ctx),
254-
StyleQuery::InParens(ref c) => c.matches(ctx),
256+
StyleQuery::Feature(ref f) => f.matches(ctx, attribute_tracker),
257+
StyleQuery::Not(ref c) => !c.matches(ctx, attribute_tracker),
258+
StyleQuery::InParens(ref c) => c.matches(ctx, attribute_tracker),
255259
StyleQuery::Operation(ref conditions, op) => {
256260
debug_assert!(!conditions.is_empty(), "We never create an empty op");
257261
match op {
258-
Operator::And => KleeneValue::any_false(conditions.iter(), |c| c.matches(ctx)),
259-
Operator::Or => KleeneValue::any(conditions.iter(), |c| c.matches(ctx)),
262+
Operator::And => KleeneValue::any_false(conditions.iter(), |c| {
263+
c.matches(ctx, attribute_tracker)
264+
}),
265+
Operator::Or => {
266+
KleeneValue::any(conditions.iter(), |c| c.matches(ctx, attribute_tracker))
267+
},
260268
}
261269
},
262270
StyleQuery::GeneralEnclosed(_) => KleeneValue::Unknown,
@@ -325,10 +333,14 @@ impl StyleFeature {
325333
Ok(Self::Plain(StyleFeaturePlain::parse(context, input)?))
326334
}
327335

328-
fn matches(&self, ctx: &computed::Context) -> KleeneValue {
336+
fn matches(
337+
&self,
338+
ctx: &computed::Context,
339+
attribute_tracker: &mut AttributeTracker,
340+
) -> KleeneValue {
329341
match self {
330-
Self::Plain(plain) => plain.matches(ctx),
331-
Self::Range(range) => range.evaluate(ctx),
342+
Self::Plain(plain) => plain.matches(ctx, attribute_tracker),
343+
Self::Range(range) => range.evaluate(ctx, attribute_tracker),
332344
}
333345
}
334346
}
@@ -401,6 +413,7 @@ impl StyleFeaturePlain {
401413
registration: &PropertyDescriptors,
402414
stylist: &Stylist,
403415
ctx: &computed::Context,
416+
attribute_tracker: &mut AttributeTracker,
404417
current_value: Option<&ComputedRegisteredValue>,
405418
) -> bool {
406419
let substitution_functions = custom_properties::ComputedSubstitutionFunctions::new(
@@ -413,8 +426,7 @@ impl StyleFeaturePlain {
413426
&substitution_functions,
414427
stylist,
415428
ctx,
416-
// FIXME: do we need to pass a real AttributeTracker for the query?
417-
&mut AttributeTracker::new_dummy(),
429+
attribute_tracker,
418430
) {
419431
Ok(sub) => sub,
420432
Err(_) => return current_value.is_none(),
@@ -439,7 +451,11 @@ impl StyleFeaturePlain {
439451
computed.as_ref() == current_value
440452
}
441453

442-
fn matches(&self, ctx: &computed::Context) -> KleeneValue {
454+
fn matches(
455+
&self,
456+
ctx: &computed::Context,
457+
attribute_tracker: &mut AttributeTracker,
458+
) -> KleeneValue {
443459
// FIXME(emilio): Confirm this is the right style to query.
444460
let stylist = ctx
445461
.builder
@@ -457,7 +473,14 @@ impl StyleFeaturePlain {
457473
} else if v.has_references() {
458474
// If there are --var() references in the query value,
459475
// try to substitute them before comparing to current.
460-
Self::substitute_and_compare(v, registration, stylist, ctx, current_value)
476+
Self::substitute_and_compare(
477+
v,
478+
registration,
479+
stylist,
480+
ctx,
481+
attribute_tracker,
482+
current_value,
483+
)
461484
} else {
462485
custom_properties::compute_variable_value(&v, registration, ctx).as_ref()
463486
== current_value
@@ -799,26 +822,27 @@ impl QueryCondition {
799822
&self,
800823
context: &computed::Context,
801824
custom: &mut CustomMediaEvaluator,
825+
attribute_tracker: &mut AttributeTracker,
802826
) -> KleeneValue {
803827
match *self {
804828
Self::Custom(ref f) => custom.matches(f, context),
805829
Self::Feature(ref f) => f.matches(context),
806830
Self::GeneralEnclosed(ref str, ref url_data) => {
807-
self.matches_general(&str, url_data, context, custom)
831+
self.matches_general(&str, url_data, context, custom, attribute_tracker)
808832
},
809-
Self::InParens(ref c) => c.matches(context, custom),
810-
Self::Not(ref c) => !c.matches(context, custom),
811-
Self::Style(ref c) => c.matches(context),
833+
Self::InParens(ref c) => c.matches(context, custom, attribute_tracker),
834+
Self::Not(ref c) => !c.matches(context, custom, attribute_tracker),
835+
Self::Style(ref c) => c.matches(context, attribute_tracker),
812836
Self::MozPref(ref c) => c.matches(context),
813837
Self::Operation(ref conditions, op) => {
814838
debug_assert!(!conditions.is_empty(), "We never create an empty op");
815839
match op {
816-
Operator::And => {
817-
KleeneValue::any_false(conditions.iter(), |c| c.matches(context, custom))
818-
},
819-
Operator::Or => {
820-
KleeneValue::any(conditions.iter(), |c| c.matches(context, custom))
821-
},
840+
Operator::And => KleeneValue::any_false(conditions.iter(), |c| {
841+
c.matches(context, custom, attribute_tracker)
842+
}),
843+
Operator::Or => KleeneValue::any(conditions.iter(), |c| {
844+
c.matches(context, custom, attribute_tracker)
845+
}),
822846
}
823847
},
824848
}
@@ -832,6 +856,7 @@ impl QueryCondition {
832856
url_data: &UrlExtraData,
833857
context: &computed::Context,
834858
custom: &mut CustomMediaEvaluator,
859+
attribute_tracker: &mut AttributeTracker,
835860
) -> KleeneValue {
836861
// This only applies (currently, at least) to container queries.
837862
if !context.in_container_query {
@@ -872,8 +897,7 @@ impl QueryCondition {
872897
&substitution_functions,
873898
stylist,
874899
context,
875-
// FIXME: do we need to pass a real AttributeTracker for the query?
876-
&mut AttributeTracker::new_dummy(),
900+
attribute_tracker,
877901
) {
878902
Ok(sub) => sub,
879903
Err(_) => return KleeneValue::Unknown,
@@ -904,7 +928,7 @@ impl QueryCondition {
904928
// If the result is still GeneralEnclosed, the query is unknown.
905929
KleeneValue::Unknown
906930
},
907-
Ok(query) => query.matches(context, custom),
931+
Ok(query) => query.matches(context, custom, attribute_tracker),
908932
Err(_) => KleeneValue::Unknown,
909933
};
910934

‎servo/components/style/queries/feature_expression.rs‎

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use crate::custom_properties::{
1212
self, ComputedSubstitutionFunctions, VariableValue as CustomVariableValue,
1313
};
1414
use crate::derives::*;
15+
use crate::dom::AttributeTracker;
1516
use crate::parser::{Parse, ParserContext};
1617
use crate::properties::CSSWideKeyword;
1718
use crate::properties_and_values::value::{ComputedValueComponent as Component, ValueInner};
@@ -921,15 +922,31 @@ impl QueryStyleRange {
921922
}
922923

923924
/// Returns whether this style-range query evaluates to true for the given context.
924-
pub fn evaluate(&self, context: &computed::Context) -> KleeneValue {
925+
pub fn evaluate(
926+
&self,
927+
context: &computed::Context,
928+
attribute_tracker: &mut AttributeTracker,
929+
) -> KleeneValue {
925930
match self {
926931
QueryStyleRange::StyleRange2 {
927932
ref value1,
928933
ref op1,
929934
ref value2,
930935
} => Self::compare_values(
931-
Self::resolve_value(value1, context, &mut PrecomputedHashSet::default()).as_ref(),
932-
Self::resolve_value(value2, context, &mut PrecomputedHashSet::default()).as_ref(),
936+
Self::resolve_value(
937+
value1,
938+
context,
939+
attribute_tracker,
940+
&mut PrecomputedHashSet::default(),
941+
)
942+
.as_ref(),
943+
Self::resolve_value(
944+
value2,
945+
context,
946+
attribute_tracker,
947+
&mut PrecomputedHashSet::default(),
948+
)
949+
.as_ref(),
933950
)
934951
.is_some_and(|c| op1.evaluate(c))
935952
.into(),
@@ -941,8 +958,18 @@ impl QueryStyleRange {
941958
ref op2,
942959
ref value3,
943960
} => {
944-
let v1 = Self::resolve_value(value1, context, &mut PrecomputedHashSet::default());
945-
let v2 = Self::resolve_value(value2, context, &mut PrecomputedHashSet::default());
961+
let v1 = Self::resolve_value(
962+
value1,
963+
context,
964+
attribute_tracker,
965+
&mut PrecomputedHashSet::default(),
966+
);
967+
let v2 = Self::resolve_value(
968+
value2,
969+
context,
970+
attribute_tracker,
971+
&mut PrecomputedHashSet::default(),
972+
);
946973
Self::compare_values(v1.as_ref(), v2.as_ref())
947974
.is_some_and(|c1| {
948975
op1.evaluate(c1)
@@ -951,6 +978,7 @@ impl QueryStyleRange {
951978
Self::resolve_value(
952979
value3,
953980
context,
981+
attribute_tracker,
954982
&mut PrecomputedHashSet::default(),
955983
)
956984
.as_ref(),
@@ -966,6 +994,7 @@ impl QueryStyleRange {
966994
fn resolve_value(
967995
value: &QueryExpressionValue,
968996
context: &computed::Context,
997+
attribute_tracker: &mut AttributeTracker,
969998
visited_set: &mut PrecomputedHashSet<DashedIdent>,
970999
) -> Option<Component> {
9711000
match value {
@@ -988,7 +1017,13 @@ impl QueryStyleRange {
9881017
// and we risk infinite recursion, so instead return None
9891018
// (i.e. the value cannot be resolved).
9901019
if visited_set.insert(ident.clone()) {
991-
Self::resolve_universal(&v.css, &v.url_data, context, visited_set)
1020+
Self::resolve_universal(
1021+
&v.css,
1022+
&v.url_data,
1023+
context,
1024+
attribute_tracker,
1025+
visited_set,
1026+
)
9921027
} else {
9931028
None
9941029
}
@@ -1013,10 +1048,16 @@ impl QueryStyleRange {
10131048
&sub_funcs,
10141049
stylist,
10151050
context,
1016-
&mut crate::dom::AttributeTracker::new_dummy(),
1051+
attribute_tracker,
10171052
)
10181053
.ok()?;
1019-
Self::resolve_universal(&substituted.css, &value.url_data, context, visited_set)
1054+
Self::resolve_universal(
1055+
&substituted.css,
1056+
&value.url_data,
1057+
context,
1058+
attribute_tracker,
1059+
visited_set,
1060+
)
10201061
},
10211062
QueryExpressionValue::Length(v) => {
10221063
Some(Component::Length(v.to_computed_value(context)))
@@ -1050,6 +1091,7 @@ impl QueryStyleRange {
10501091
css_text: &str,
10511092
url_data: &UrlExtraData,
10521093
context: &computed::Context,
1094+
attribute_tracker: &mut AttributeTracker,
10531095
visited_set: &mut PrecomputedHashSet<DashedIdent>,
10541096
) -> Option<Component> {
10551097
let parser_context = ParserContext::new(
@@ -1065,7 +1107,9 @@ impl QueryStyleRange {
10651107
let mut input = ParserInput::new(css_text);
10661108
QueryExpressionValue::parse_for_style_range(&parser_context, &mut Parser::new(&mut input))
10671109
.ok()
1068-
.and_then(|parsed| Self::resolve_value(&parsed, context, visited_set))
1110+
.and_then(|parsed| {
1111+
Self::resolve_value(&parsed, context, attribute_tracker, visited_set)
1112+
})
10691113
}
10701114

10711115
fn compare_values(value1: Option<&Component>, value2: Option<&Component>) -> Option<Ordering> {

‎servo/components/style/stylesheets/container_rule.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
99
use crate::computed_value_flags::ComputedValueFlags;
1010
use crate::derives::*;
11-
use crate::dom::TElement;
11+
use crate::dom::{AttributeTracker, TElement};
1212
use crate::logical_geometry::{LogicalSize, WritingMode};
1313
use crate::parser::ParserContext;
1414
use crate::properties::ComputedValues;
@@ -30,8 +30,8 @@ use malloc_size_of::{MallocSizeOfOps, MallocUnconditionalShallowSizeOf};
3030
use selectors::kleene_value::KleeneValue;
3131
use servo_arc::Arc;
3232
use std::fmt::{self, Write};
33-
use style_traits::{CssStringWriter, CssWriter, ParseError, StyleParseErrorKind, ToCss};
3433
use style_traits::arc_slice::ArcSlice;
34+
use style_traits::{CssStringWriter, CssWriter, ParseError, StyleParseErrorKind, ToCss};
3535

3636
/// A container rule.
3737
#[derive(Debug, ToShmem)]
@@ -155,12 +155,12 @@ where
155155
impl ContainerCondition {
156156
/// Get the name of this condition.
157157
#[inline]
158-
pub fn name(&self) -> &ContainerName{
158+
pub fn name(&self) -> &ContainerName {
159159
&self.name
160160
}
161161
/// Get the query condition of this condition
162162
#[inline]
163-
pub fn query_condition(&self) -> Option<&QueryCondition>{
163+
pub fn query_condition(&self) -> Option<&QueryCondition> {
164164
self.condition.as_ref()
165165
}
166166
/// Parse a container condition.
@@ -298,13 +298,18 @@ impl ContainerCondition {
298298
let size_query_container_lookup = ContainerSizeQuery::for_element(
299299
container, /* known_parent_style = */ None, /* is_pseudo = */ false,
300300
);
301+
let mut attribute_tracker = AttributeTracker::new(&container);
301302
Context::for_container_query_evaluation(
302303
stylist.device(),
303304
Some(stylist),
304305
Some(info),
305306
size_query_container_lookup,
306307
|context| {
307-
let matches = condition.matches(context, &mut CustomMediaEvaluator::none());
308+
let matches = condition.matches(
309+
context,
310+
&mut CustomMediaEvaluator::none(),
311+
&mut attribute_tracker,
312+
);
308313
let flags = context.style().flags();
309314
if flags.contains(ComputedValueFlags::USES_VIEWPORT_UNITS) {
310315
// TODO(emilio): Might need something similar to improve

0 commit comments

Comments
 (0)