Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Commit fe41e97

Browse files
emilioemilio
committed
Bug 1675514 - Fix declaration block serialization and logical properties. r=boris
As per w3c/csswg-drafts#3030. Also remove a system font special-case. It's generally handled in shorthands/font.mako.rs, and when it's not, it's really the same issue as w3c/csswg-drafts#2515. Browsers are all over the place for the system font case, so I think it's fine. Differential Revision: https://phabricator.services.mozilla.com/D96045
1 parent 499ac31 commit fe41e97

4 files changed

Lines changed: 157 additions & 96 deletions

File tree

‎layout/style/test/test_system_font_serialization.html‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,15 +42,13 @@
4242
is(e.style.font, "menu", "font getter returns value");
4343

4444
e.style.fontFamily = "inherit";
45-
is(e.style.cssText, "font: menu; font-family: inherit;", "serialize system font and font-family");
4645
is(e.style.font, "", "font getter should be empty");
4746

4847
e.style.font = "message-box";
4948
is(e.style.cssText, "font: message-box;", "serialize system font alone");
5049
is(e.style.font, "message-box", "font getter returns value");
5150

5251
e.setAttribute("style", "font-weight:bold;font:caption;line-height:3;");
53-
is(e.style.cssText, "font: caption; line-height: 3;", "serialize system font and font-family");
5452
is(e.style.font, "", "font getter should be empty");
5553

5654
e.setAttribute("style", "font: menu; font-weight: -moz-use-system-font");

‎servo/components/style/properties/declaration_block.rs‎

Lines changed: 113 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,7 @@ impl PropertyDeclarationBlock {
930930
let mut already_serialized = NonCustomPropertyIdSet::new();
931931

932932
// Step 3
933-
for (declaration, importance) in self.declaration_importance_iter() {
933+
'declaration_loop: for (declaration, importance) in self.declaration_importance_iter() {
934934
// Step 3.1
935935
let property = declaration.id();
936936
let longhand_id = match property {
@@ -956,11 +956,7 @@ impl PropertyDeclarationBlock {
956956
continue;
957957
}
958958

959-
// Step 3.3
960-
// Step 3.3.1 is done by checking already_serialized while
961-
// iterating below.
962-
963-
// Step 3.3.2
959+
// Steps 3.3 & 3.4
964960
for shorthand in longhand_id.shorthands() {
965961
// We already attempted to serialize this shorthand before.
966962
if already_serialized.contains(shorthand.into()) {
@@ -972,74 +968,105 @@ impl PropertyDeclarationBlock {
972968
continue;
973969
}
974970

975-
// Substep 2 & 3
971+
// Step 3.3.1:
972+
// Let longhands be an array consisting of all CSS
973+
// declarations in declaration block’s declarations that
974+
// that are not in already serialized and have a property
975+
// name that maps to one of the shorthand properties in
976+
// shorthands.
977+
let longhands = {
978+
// TODO(emilio): This could just index in an array if we
979+
// remove pref-controlled longhands.
980+
let mut ids = LonghandIdSet::new();
981+
for longhand in shorthand.longhands() {
982+
ids.insert(longhand);
983+
}
984+
ids
985+
};
986+
987+
// Step 3.4.2
988+
// If all properties that map to shorthand are not present
989+
// in longhands, continue with the steps labeled shorthand
990+
// loop.
991+
if !self.longhands.contains_all(&longhands) {
992+
continue;
993+
}
994+
995+
// Step 3.4.3:
996+
// Let current longhands be an empty array.
976997
let mut current_longhands = SmallVec::<[_; 10]>::new();
998+
let mut logical_groups = LogicalGroupSet::new();
999+
let mut saw_one = false;
1000+
let mut logical_mismatch = false;
1001+
let mut seen = LonghandIdSet::new();
9771002
let mut important_count = 0;
978-
let mut found_system = None;
9791003

980-
let is_system_font = shorthand == ShorthandId::Font &&
981-
self.declarations.iter().any(|l| match l.id() {
982-
PropertyDeclarationId::Longhand(id) => {
983-
if already_serialized.contains(id.into()) {
984-
return false;
985-
}
1004+
// Step 3.4.4:
1005+
// Append all CSS declarations in longhands that have a
1006+
// property name that maps to shorthand to current longhands.
1007+
for (declaration, importance) in self.declaration_importance_iter() {
1008+
let longhand = match declaration.id() {
1009+
PropertyDeclarationId::Longhand(id) => id,
1010+
PropertyDeclarationId::Custom(..) => continue,
1011+
};
9861012

987-
l.get_system().is_some()
988-
},
989-
PropertyDeclarationId::Custom(..) => {
990-
debug_assert!(l.get_system().is_none());
991-
false
992-
},
993-
});
994-
995-
if is_system_font {
996-
for (longhand, importance) in self.declaration_importance_iter() {
997-
if longhand.get_system().is_some() || longhand.is_default_line_height() {
998-
current_longhands.push(longhand);
999-
if found_system.is_none() {
1000-
found_system = longhand.get_system();
1013+
if longhands.contains(longhand) {
1014+
saw_one = true;
1015+
if importance.important() {
1016+
important_count += 1;
1017+
}
1018+
current_longhands.push(declaration);
1019+
if shorthand != ShorthandId::All {
1020+
// All is special because it contains both physical
1021+
// and logical longhands.
1022+
if let Some(g) = longhand.logical_group() {
1023+
logical_groups.insert(g);
10011024
}
1002-
if importance.important() {
1003-
important_count += 1;
1025+
seen.insert(longhand);
1026+
if seen == longhands {
1027+
break;
10041028
}
10051029
}
1006-
}
1007-
} else {
1008-
let mut contains_all_longhands = true;
1009-
for longhand in shorthand.longhands() {
1010-
match self.get(PropertyDeclarationId::Longhand(longhand)) {
1011-
Some((declaration, importance)) => {
1012-
current_longhands.push(declaration);
1013-
if importance.important() {
1014-
important_count += 1;
1015-
}
1016-
},
1017-
None => {
1018-
contains_all_longhands = false;
1030+
} else if saw_one {
1031+
if let Some(g) = longhand.logical_group() {
1032+
if logical_groups.contains(g) {
1033+
logical_mismatch = true;
10191034
break;
1020-
},
1035+
}
10211036
}
10221037
}
1023-
1024-
// Substep 1:
1025-
if !contains_all_longhands {
1026-
continue;
1027-
}
10281038
}
10291039

1030-
// Substep 4
1040+
// 3.4.5:
1041+
// If there is one or more CSS declarations in current
1042+
// longhands have their important flag set and one or more
1043+
// with it unset, continue with the steps labeled shorthand
1044+
// loop.
10311045
let is_important = important_count > 0;
10321046
if is_important && important_count != current_longhands.len() {
10331047
continue;
10341048
}
1049+
1050+
// 3.4.6:
1051+
// If there’s any declaration in declaration block in between
1052+
// the first and the last longhand in current longhands which
1053+
// belongs to the same logical property group, but has a
1054+
// different mapping logic as any of the longhands in current
1055+
// longhands, and is not in current longhands, continue with
1056+
// the steps labeled shorthand loop.
1057+
if logical_mismatch {
1058+
continue;
1059+
}
1060+
10351061
let importance = if is_important {
10361062
Importance::Important
10371063
} else {
10381064
Importance::Normal
10391065
};
10401066

1041-
// Substep 5 - Let value be the result of invoking serialize
1042-
// a CSS value of current longhands.
1067+
// 3.4.7:
1068+
// Let value be the result of invoking serialize a CSS value
1069+
// of current longhands.
10431070
let appendable_value = match shorthand
10441071
.get_shorthand_appendable_value(current_longhands.iter().cloned())
10451072
{
@@ -1050,32 +1077,17 @@ impl PropertyDeclarationBlock {
10501077
// We avoid re-serializing if we're already an
10511078
// AppendableValue::Css.
10521079
let mut v = CssString::new();
1053-
let value = match (appendable_value, found_system) {
1054-
(
1055-
AppendableValue::Css {
1056-
css,
1057-
with_variables,
1058-
},
1059-
_,
1060-
) => {
1080+
let value = match appendable_value {
1081+
AppendableValue::Css { css, with_variables } => {
10611082
debug_assert!(!css.is_empty());
1062-
AppendableValue::Css {
1063-
css: css,
1064-
with_variables: with_variables,
1065-
}
1083+
AppendableValue::Css { css, with_variables }
10661084
},
1067-
#[cfg(feature = "gecko")]
1068-
(_, Some(sys)) => {
1069-
sys.to_css(&mut CssWriter::new(&mut v))?;
1070-
AppendableValue::Css {
1071-
css: CssStringBorrow::from(&v),
1072-
with_variables: false,
1073-
}
1074-
},
1075-
(other, _) => {
1085+
other => {
10761086
append_declaration_value(&mut v, other)?;
10771087

1078-
// Substep 6
1088+
// 3.4.8:
1089+
// If value is the empty string, continue with the
1090+
// steps labeled shorthand loop.
10791091
if v.is_empty() {
10801092
continue;
10811093
}
@@ -1087,7 +1099,15 @@ impl PropertyDeclarationBlock {
10871099
},
10881100
};
10891101

1090-
// Substeps 7 and 8
1102+
// 3.4.9:
1103+
// Let serialized declaration be the result of invoking
1104+
// serialize a CSS declaration with property name shorthand,
1105+
// value value, and the important flag set if the CSS
1106+
// declarations in current longhands have their important
1107+
// flag set.
1108+
//
1109+
// 3.4.10:
1110+
// Append serialized declaration to list.
10911111
append_serialization::<Cloned<slice::Iter<_>>, _>(
10921112
dest,
10931113
&shorthand,
@@ -1096,6 +1116,9 @@ impl PropertyDeclarationBlock {
10961116
&mut is_first_serialization,
10971117
)?;
10981118

1119+
// 3.4.11:
1120+
// Append the property names of all items of current
1121+
// longhands to already serialized.
10991122
for current_longhand in &current_longhands {
11001123
let longhand_id = match current_longhand.id() {
11011124
PropertyDeclarationId::Longhand(id) => id,
@@ -1106,24 +1129,21 @@ impl PropertyDeclarationBlock {
11061129
already_serialized.insert(longhand_id.into());
11071130
}
11081131

1109-
// FIXME(https://github.com/w3c/csswg-drafts/issues/1774)
1110-
// The specification does not include an instruction to abort
1111-
// the shorthand loop at this point, but doing so both matches
1112-
// Gecko and makes sense since shorthands are checked in
1113-
// preferred order.
1114-
break;
1115-
}
1116-
1117-
// Step 3.3.4
1118-
if already_serialized.contains(longhand_id.into()) {
1119-
continue;
1132+
// 3.4.12:
1133+
// Continue with the steps labeled declaration loop.
1134+
continue 'declaration_loop;
11201135
}
11211136

1122-
// Steps 3.3.5, 3.3.6 & 3.3.7
1123-
// Need to specify an iterator type here even though it’s unused to work around
1124-
// "error: unable to infer enough type information about `_`;
1125-
// type annotations or generic parameter binding required [E0282]"
1126-
// Use the same type as earlier call to reuse generated code.
1137+
// Steps 3.5, 3.6 & 3.7:
1138+
// Let value be the result of invoking serialize a CSS value of
1139+
// declaration.
1140+
//
1141+
// Let serialized declaration be the result of invoking
1142+
// serialize a CSS declaration with property name property,
1143+
// value value, and the important flag set if declaration has
1144+
// its important flag set.
1145+
//
1146+
// Append serialized declaration to list.
11271147
append_serialization::<Cloned<slice::Iter<_>>, _>(
11281148
dest,
11291149
&property,
@@ -1132,7 +1152,8 @@ impl PropertyDeclarationBlock {
11321152
&mut is_first_serialization,
11331153
)?;
11341154

1135-
// Step 3.3.8
1155+
// Step 3.8:
1156+
// Append property to already serialized.
11361157
already_serialized.insert(longhand_id.into());
11371158
}
11381159

‎servo/components/style/properties/properties.mako.rs‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -792,14 +792,44 @@ static ${name}: LonghandIdSet = LonghandIdSet {
792792
/// A group for properties which may override each other
793793
/// via logical resolution.
794794
#[derive(Clone, Copy, Eq, Hash, PartialEq)]
795+
#[repr(u8)]
795796
pub enum LogicalGroup {
796-
% for group in sorted(logical_groups.keys()):
797+
% for i, group in enumerate(logical_groups.keys()):
797798
/// ${group}
798-
${to_camel_case(group)},
799+
${to_camel_case(group)} = ${i},
799800
% endfor
800801
}
801802

802803

804+
/// A set of logical groups.
805+
#[derive(Clone, Copy, Debug, Default, MallocSizeOf, PartialEq)]
806+
pub struct LogicalGroupSet {
807+
storage: [u32; (${len(logical_groups)} - 1 + 32) / 32]
808+
}
809+
810+
impl LogicalGroupSet {
811+
/// Creates an empty `NonCustomPropertyIdSet`.
812+
pub fn new() -> Self {
813+
Self {
814+
storage: Default::default(),
815+
}
816+
}
817+
818+
/// Return whether the given group is in the set
819+
#[inline]
820+
pub fn contains(&self, g: LogicalGroup) -> bool {
821+
let bit = g as usize;
822+
(self.storage[bit / 32] & (1 << (bit % 32))) != 0
823+
}
824+
825+
/// Insert a group the set.
826+
#[inline]
827+
pub fn insert(&mut self, g: LogicalGroup) {
828+
let bit = g as usize;
829+
self.storage[bit / 32] |= 1 << (bit % 32);
830+
}
831+
}
832+
803833
/// A set of longhand properties
804834
#[derive(Clone, Copy, Debug, Default, MallocSizeOf, PartialEq)]
805835
pub struct LonghandIdSet {

‎testing/web-platform/tests/css/cssom/cssstyledeclaration-csstext.html‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,18 @@
9797

9898
}, 'invalid property does not appear');
9999

100+
test(function(){
101+
var style = newElm().style;
102+
style.cssText = "margin: 10px; margin-inline: 10px; margin-block: 10px; margin-inline-end: 10px; margin-bottom: 10px;";
103+
assert_equals(style.cssText, "margin-top: 10px; margin-right: 10px; margin-left: 10px; margin-inline-start: 10px; margin-block: 10px; margin-inline-end: 10px; margin-bottom: 10px;");
104+
}, 'Shorthands aren\'t serialized if there are other properties with different logical groups in between');
105+
106+
test(function(){
107+
var style = newElm().style;
108+
style.cssText = "margin-top: 10px; margin-left: 10px; margin-right: 10px; margin-bottom: 10px; margin-inline-start: 10px; margin-inline-end: 10px; margin-block-start: 10px; margin-block-end: 10px;";
109+
assert_equals(style.cssText, "margin: 10px; margin-inline: 10px; margin-block: 10px;");
110+
}, 'Shorthands _are_ serialized if there are no other properties with different logical groups in between');
111+
100112
// https://github.com/w3c/csswg-drafts/issues/1033
101113
test(function() {
102114
var elm = newElm();

0 commit comments

Comments
 (0)