Skip to content

Commit

Permalink
Bug 1850170 - Don't use negate-values-and-swap-bounds to negate CSS c…
Browse files Browse the repository at this point in the history
…lamp() when min > max, because the result is order-dependent. r=emilio

This fixes the incorrect result when negating a clamp() where the min value is greater than max.
Added some extra tests to clamp-length-computed.html; the last example fails in Gecko without
the patch here.

Differential Revision: https://phabricator.services.mozilla.com/D187113
  • Loading branch information
jfkthame committed Aug 31, 2023
1 parent a591ed2 commit 66cb47b
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 4 deletions.
12 changes: 8 additions & 4 deletions servo/components/style/values/generics/calc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,11 +511,15 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
ref mut center,
ref mut max,
} => {
min.negate();
center.negate();
max.negate();
if min <= max {
min.negate();
center.negate();
max.negate();

mem::swap(min, max);
mem::swap(min, max);
} else {
wrap_self_in_negate(self);
}
},
CalcNode::Round {
ref mut value,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,17 @@
// so MIN wins over MAX if they are in the wrong order.
test_length_equals('clamp(30px, 100px, 20px)', '30px');

// also test with negative values
test_length_equals('clamp(-30px, -20px, -10px)', '-20px');
test_length_equals('clamp(-30px, -100px, -10px)', '-30px');
test_length_equals('clamp(-30px, 100px, -10px)', '-10px');
test_length_equals('clamp(-10px, 100px, -30px)', '-10px');
test_length_equals('clamp(-10px, -100px, -30px)', '-10px');

// and negating the result of clamp
test_length_equals('calc(0px + clamp(10px, 20px, 30px))', '20px');
test_length_equals('calc(0px - clamp(10px, 20px, 30px))', '-20px');
test_length_equals('calc(0px + clamp(30px, 100px, 20px))', '30px');
test_length_equals('calc(0px - clamp(30px, 100px, 20px))', '-30px');

</script>
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,32 @@
test_valid_length('clamp(1px, 2px, 3px)', 'calc(2px)');
test_valid_length('clamp(1px, 2px, clamp(2px, 3px, 4px))', 'calc(2px)');

test_valid_length('clamp(10px, 5px, 30px)', 'calc(10px)');
test_valid_length('clamp(10px, 35px, 30px)', 'calc(30px)');
test_valid_length('clamp(10px, 35px , 30px)', 'calc(30px)');
test_valid_length('clamp(10px, 35px /*foo*/, 30px)', 'calc(30px)');
test_valid_length('clamp(10px /* foo */ , 35px, 30px)', 'calc(30px)');
test_valid_length('clamp(10px , 35px, 30px)', 'calc(30px)');

// clamp(MIN, VAL, MAX) is identical to max(MIN, min(VAL, MAX)),
// so MIN wins over MAX if they are in the wrong order.
test_valid_length('clamp(30px, 100px, 20px)', 'calc(30px)');

// also test with negative values
test_valid_length('clamp(-30px, -20px, -10px)', 'calc(-20px)');
test_valid_length('clamp(-30px, -100px, -10px)', 'calc(-30px)');
test_valid_length('clamp(-30px, 100px, -10px)', 'calc(-10px)');
test_valid_length('clamp(-10px, 100px, -30px)', 'calc(-10px)');
test_valid_length('clamp(-10px, -100px, -30px)', 'calc(-10px)');

// and negating the result of clamp
test_valid_length('calc(0px + clamp(10px, 20px, 30px))', 'calc(20px)');
test_valid_length('calc(0px - clamp(10px, 20px, 30px))', 'calc(-20px)');
test_valid_length('calc(0px + clamp(30px, 100px, 20px))', 'calc(30px)');
test_valid_length('calc(0px - clamp(30px, 100px, 20px))', 'calc(-30px)');

// mixed units that can't be resolved until computed-value time
test_valid_length('clamp(1px, 1em, 1vh)', 'clamp(1px, 1em, 1vh)');
test_valid_length('calc(0px + clamp(1px, 1em, 1vh))', 'calc(0px + clamp(1px, 1em, 1vh))');
test_valid_length('calc(0px - clamp(1px, 1em, 1vh))', 'calc(0px - clamp(1px, 1em, 1vh))');
</script>

0 comments on commit 66cb47b

Please sign in to comment.