From e097b8a125f3af5306f73e6262fe363f3b4e1aa9 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Tue, 14 Apr 2020 21:53:36 +0000 Subject: [PATCH 1/2] Bug 1628484 - Remove assertions that don't always hold. r=kvark These assertions don't hold in some perfectly legitimate cases, such as when items have both top and bottom sticky behaviours. The actual behaviour still seems ok, so let's just drop the assertions. Differential Revision: https://phabricator.services.mozilla.com/D70459 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/259c147efa83074869ce93c3053b4ecbd80863d6 --- webrender/src/spatial_node.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/webrender/src/spatial_node.rs b/webrender/src/spatial_node.rs index c3e58e97f8..a6758224fb 100644 --- a/webrender/src/spatial_node.rs +++ b/webrender/src/spatial_node.rs @@ -485,7 +485,6 @@ impl SpatialNode { sticky_offset.y = top_viewport_edge - sticky_rect.min_y(); sticky_offset.y = sticky_offset.y.max(-info.previously_applied_offset.y); } - debug_assert!(sticky_offset.y + info.previously_applied_offset.y >= 0.0); } // If we don't have a sticky-top offset (sticky_offset.y + info.previously_applied_offset.y @@ -505,7 +504,6 @@ impl SpatialNode { sticky_offset.y = bottom_viewport_edge - sticky_rect.max_y(); sticky_offset.y = sticky_offset.y.min(-info.previously_applied_offset.y); } - debug_assert!(sticky_offset.y + info.previously_applied_offset.y <= 0.0); } } @@ -519,7 +517,6 @@ impl SpatialNode { sticky_offset.x = left_viewport_edge - sticky_rect.min_x(); sticky_offset.x = sticky_offset.x.max(-info.previously_applied_offset.x); } - debug_assert!(sticky_offset.x + info.previously_applied_offset.x >= 0.0); } if sticky_offset.x + info.previously_applied_offset.x <= 0.0 { @@ -532,7 +529,6 @@ impl SpatialNode { sticky_offset.x = right_viewport_edge - sticky_rect.max_x(); sticky_offset.x = sticky_offset.x.min(-info.previously_applied_offset.x); } - debug_assert!(sticky_offset.x + info.previously_applied_offset.x <= 0.0); } } From 686307b264e14dda450d0d42aa0bba734d77ed08 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Tue, 14 Apr 2020 21:53:46 +0000 Subject: [PATCH 2/2] Bug 1629521 - Fix WR handling of items with both top and bottom sticky ranges. r=kvark The WR code that computed the sticky_offset didn't properly combine the offsets from the top- and bottom- sticky calculations if an item had both. This patch fixes the calculation, which makes the remaining test failure (in the configuration without any dynamic toolbar) pass. Depends on D70679 Differential Revision: https://phabricator.services.mozilla.com/D70680 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/2c065921de099b97532775e1aca5973edd5512b2 --- webrender/src/spatial_node.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/webrender/src/spatial_node.rs b/webrender/src/spatial_node.rs index a6758224fb..2000eec9e8 100644 --- a/webrender/src/spatial_node.rs +++ b/webrender/src/spatial_node.rs @@ -464,7 +464,7 @@ impl SpatialNode { // between the scrolled content and unscrolled viewport we adjust the viewport's // position by the scroll offset in order to work with their relative positions on the // page. - let sticky_rect = info.frame_rect.translate(*viewport_scroll_offset); + let mut sticky_rect = info.frame_rect.translate(*viewport_scroll_offset); let mut sticky_offset = LayoutVector2D::zero(); if let Some(margin) = info.margins.top { @@ -489,19 +489,28 @@ impl SpatialNode { // If we don't have a sticky-top offset (sticky_offset.y + info.previously_applied_offset.y // == 0), or if we have a previously-applied bottom offset (previously_applied_offset.y < 0) - // then we check for handling the bottom margin case. + // then we check for handling the bottom margin case. Note that the "don't have a sticky-top + // offset" case includes the case where we *had* a sticky-top offset but we reduced it to + // zero in the above block. if sticky_offset.y + info.previously_applied_offset.y <= 0.0 { if let Some(margin) = info.margins.bottom { + // If sticky_offset.y is nonzero that means we must have set it + // in the sticky-top handling code above, so this item must have + // both top and bottom sticky margins. We adjust the item's rect + // by the top-sticky offset, and then combine any offset from + // the bottom-sticky calculation into sticky_offset below. + sticky_rect.origin.y += sticky_offset.y; + // Same as the above case, but inverted for bottom-sticky items. Here // we adjust items upwards, resulting in a negative sticky_offset.y, // or reduce the already-present upward adjustment, resulting in a positive // sticky_offset.y. let bottom_viewport_edge = viewport_rect.max_y() - margin; if sticky_rect.max_y() > bottom_viewport_edge { - sticky_offset.y = bottom_viewport_edge - sticky_rect.max_y(); + sticky_offset.y += bottom_viewport_edge - sticky_rect.max_y(); } else if info.previously_applied_offset.y < 0.0 && sticky_rect.max_y() < bottom_viewport_edge { - sticky_offset.y = bottom_viewport_edge - sticky_rect.max_y(); + sticky_offset.y += bottom_viewport_edge - sticky_rect.max_y(); sticky_offset.y = sticky_offset.y.min(-info.previously_applied_offset.y); } } @@ -521,12 +530,13 @@ impl SpatialNode { if sticky_offset.x + info.previously_applied_offset.x <= 0.0 { if let Some(margin) = info.margins.right { + sticky_rect.origin.x += sticky_offset.x; let right_viewport_edge = viewport_rect.max_x() - margin; if sticky_rect.max_x() > right_viewport_edge { - sticky_offset.x = right_viewport_edge - sticky_rect.max_x(); + sticky_offset.x += right_viewport_edge - sticky_rect.max_x(); } else if info.previously_applied_offset.x < 0.0 && sticky_rect.max_x() < right_viewport_edge { - sticky_offset.x = right_viewport_edge - sticky_rect.max_x(); + sticky_offset.x += right_viewport_edge - sticky_rect.max_x(); sticky_offset.x = sticky_offset.x.min(-info.previously_applied_offset.x); } }