From 908c952ab01e9cd7e99d26732cb7b37314aff241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 21 May 2023 22:00:09 +0200 Subject: [PATCH] style: Don't simplify percentages that resolve to lengths in min/max/clamp Those can't be ordered at specified / computed value time, since the percentage basis could be negative. Needs tests of course, running through try atm. Differential Revision: https://phabricator.services.mozilla.com/D115591 --- .../style/values/computed/length_percentage.rs | 17 +++-------------- components/style/values/specified/calc.rs | 6 +++++- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/components/style/values/computed/length_percentage.rs b/components/style/values/computed/length_percentage.rs index 098b1abfafc..cea2466ce6f 100644 --- a/components/style/values/computed/length_percentage.rs +++ b/components/style/values/computed/length_percentage.rs @@ -607,22 +607,11 @@ impl CalcLengthPercentageLeaf { impl PartialOrd for CalcLengthPercentageLeaf { fn partial_cmp(&self, other: &Self) -> Option { use self::CalcLengthPercentageLeaf::*; - - if std::mem::discriminant(self) != std::mem::discriminant(other) { - return None; - } - + // NOTE: Percentages can't be compared reasonably here because the + // percentage basis might be negative, see bug 1709018. match (self, other) { (&Length(ref one), &Length(ref other)) => one.partial_cmp(other), - (&Percentage(ref one), &Percentage(ref other)) => one.partial_cmp(other), - _ => { - match *self { - Length(..) | Percentage(..) => {}, - } - unsafe { - debug_unreachable!("Forgot a branch?"); - } - }, + _ => None, } } } diff --git a/components/style/values/specified/calc.rs b/components/style/values/specified/calc.rs index d5733a6ce4a..a22d6fd5f18 100644 --- a/components/style/values/specified/calc.rs +++ b/components/style/values/specified/calc.rs @@ -107,8 +107,12 @@ impl PartialOrd for Leaf { } match (self, other) { + // NOTE: Percentages can't be compared reasonably here because the + // percentage basis might be negative, see bug 1709018. + // Conveniently, we only use this for (for raw + // percentages, we go through resolve()). + (&Percentage(..), &Percentage(..)) => None, (&Length(ref one), &Length(ref other)) => one.partial_cmp(other), - (&Percentage(ref one), &Percentage(ref other)) => one.partial_cmp(other), (&Angle(ref one), &Angle(ref other)) => one.degrees().partial_cmp(&other.degrees()), (&Time(ref one), &Time(ref other)) => one.seconds().partial_cmp(&other.seconds()), (&Number(ref one), &Number(ref other)) => one.partial_cmp(other),