Bug 1498639 - Give WR the id of the scroll frame perspective scrolls relative to, and compute the right transform based on that. r=kats,kvark

I think this is as clean as it can get.

Differential Revision: https://phabricator.services.mozilla.com/D17848

gfx/layers/wr/StackingContextHelper.cpp

@@ -36,7 +36,7 @@ StackingContextHelper::StackingContextHelper(
   gfx::Matrix transform2d;
   if (aParams.mBoundTransform &&
       aParams.mBoundTransform->CanDraw2D(&transform2d) &&
-      aParams.reference_frame_kind != wr::ReferenceFrameKind::Perspective &&
+      aParams.reference_frame_kind != wr::WrReferenceFrameKind::Perspective &&
       !aParentSC.mIsPreserve3D) {
     mInheritedTransform = transform2d * aParentSC.mInheritedTransform;
 

gfx/webrender_bindings/WebRenderAPI.h

@@ -317,7 +317,8 @@ struct MOZ_STACK_CLASS StackingContextParams : public WrStackingContextParams {
                                 nullptr,
                                 nullptr,
                                 wr::TransformStyle::Flat,
-                                wr::ReferenceFrameKind::Transform,
+                                wr::WrReferenceFrameKind::Transform,
+                                nullptr,
                                 /* is_backface_visible = */ true,
                                 /* cache_tiles = */ false,
                                 wr::MixBlendMode::Normal} {}

gfx/webrender_bindings/src/bindings.rs

@@ -1895,6 +1895,13 @@ pub extern "C" fn wr_dp_clear_save(state: &mut WrState) {
     state.frame_builder.dl_builder.clear_save();
 }
 
+#[repr(u8)]
+#[derive(PartialEq, Eq, Debug)]
+pub enum WrReferenceFrameKind {
+    Transform,
+    Perspective,
+}
+
 /// IMPORTANT: If you add fields to this struct, you need to also add initializers
 /// for those fields in WebRenderAPI.h.
 #[repr(C)]
@@ -1903,7 +1910,8 @@ pub struct WrStackingContextParams {
     pub animation: *const WrAnimationProperty,
     pub opacity: *const f32,
     pub transform_style: TransformStyle,
-    pub reference_frame_kind: ReferenceFrameKind,
+    pub reference_frame_kind: WrReferenceFrameKind,
+    pub scrolling_relative_to: *const u64,
     pub is_backface_visible: bool,
     /// True if picture caching should be enabled for this stacking context.
     pub cache_tiles: bool,
@@ -1974,12 +1982,26 @@ pub extern "C" fn wr_dp_push_stacking_context(
     // This is resolved into proper `Maybe<WrSpatialId>` inside `WebRenderAPI::PushStackingContext`.
     let mut result = WrSpatialId { id: 0 };
     if let Some(transform_binding) = transform_binding {
+        let scrolling_relative_to = match unsafe { params.scrolling_relative_to.as_ref() } {
+            Some(scroll_id) => {
+                debug_assert_eq!(params.reference_frame_kind, WrReferenceFrameKind::Perspective);
+                Some(ExternalScrollId(*scroll_id, state.pipeline_id))
+            }
+            None => None,
+        };
+
+        let reference_frame_kind = match params.reference_frame_kind {
+            WrReferenceFrameKind::Transform => ReferenceFrameKind::Transform,
+            WrReferenceFrameKind::Perspective => ReferenceFrameKind::Perspective {
+                scrolling_relative_to,
+            },
+        };
         wr_spatial_id = state.frame_builder.dl_builder.push_reference_frame(
             &bounds,
             wr_spatial_id,
             params.transform_style,
             transform_binding,
-            params.reference_frame_kind,
+            reference_frame_kind,
         );
 
         bounds.origin = LayoutPoint::zero();

gfx/wr/webrender/src/clip_scroll_tree.rs

@@ -309,12 +309,13 @@ impl ClipScrollTree {
         self.nodes_to_update.push((root_node_index, state));
 
         while let Some((node_index, mut state)) = self.nodes_to_update.pop() {
-            let node = match self.spatial_nodes.get_mut(node_index.0 as usize) {
+            let (previous, following) = self.spatial_nodes.split_at_mut(node_index.0 as usize);
+            let node = match following.get_mut(0) {
                 Some(node) => node,
                 None => continue,
             };
 
-            node.update(&mut state, &mut self.coord_systems, scene_properties);
+            node.update(&mut state, &mut self.coord_systems, scene_properties, &*previous);
             if let Some(ref mut palette) = transform_palette {
                 node.push_gpu_data(palette, node_index);
             }
@@ -526,7 +527,7 @@ fn add_reference_frame(
         parent,
         TransformStyle::Preserve3D,
         PropertyBinding::Value(transform),
-        ReferenceFrameKind::Perspective,
+        ReferenceFrameKind::Transform,
         origin_in_parent_reference_frame,
         PipelineId::dummy(),
     )

gfx/wr/webrender/src/spatial_node.rs

@@ -69,6 +69,36 @@ pub struct SpatialNode {
     pub coordinate_system_relative_scale_offset: ScaleOffset,
 }
 
+fn compute_offset_from(
+    mut current: Option<SpatialNodeIndex>,
+    external_id: ExternalScrollId,
+    previous_spatial_nodes: &[SpatialNode],
+) -> LayoutVector2D {
+    let mut offset = LayoutVector2D::zero();
+    while let Some(parent_index) = current {
+        let ancestor = &previous_spatial_nodes[parent_index.0 as usize];
+        match ancestor.node_type {
+            SpatialNodeType::ReferenceFrame(..) => {
+                // FIXME(emilio, bug 1523436): Breaking here is technically
+                // wrong and can happen if the perspective frame is transformed
+                // as well.
+                break;
+            },
+            SpatialNodeType::ScrollFrame(ref info) => {
+                if info.external_id == Some(external_id) {
+                    break;
+                }
+                offset += info.offset;
+            },
+            SpatialNodeType::StickyFrame(ref info) => {
+                offset += info.current_offset;
+            },
+        }
+        current = ancestor.parent;
+    }
+    offset
+}
+
 impl SpatialNode {
     pub fn new(
         pipeline_id: PipelineId,
@@ -221,6 +251,7 @@ impl SpatialNode {
         state: &mut TransformUpdateState,
         coord_systems: &mut Vec<CoordinateSystem>,
         scene_properties: &SceneProperties,
+        previous_spatial_nodes: &[SpatialNode],
     ) {
         // If any of our parents was not rendered, we are not rendered either and can just
         // quit here.
@@ -229,7 +260,7 @@ impl SpatialNode {
             return;
         }
 
-        self.update_transform(state, coord_systems, scene_properties);
+        self.update_transform(state, coord_systems, scene_properties, previous_spatial_nodes);
         self.transform_kind = self.world_content_transform.kind();
 
         // If this node is a reference frame, we check if it has a non-invertible matrix.
@@ -249,6 +280,7 @@ impl SpatialNode {
         state: &mut TransformUpdateState,
         coord_systems: &mut Vec<CoordinateSystem>,
         scene_properties: &SceneProperties,
+        previous_spatial_nodes: &[SpatialNode],
     ) {
         match self.node_type {
             SpatialNodeType::ReferenceFrame(ref mut info) => {
@@ -260,13 +292,20 @@ impl SpatialNode {
                 // Do a change-basis operation on the perspective matrix using
                 // the scroll offset.
                 let source_transform = match info.kind {
-                    ReferenceFrameKind::Perspective => {
+                    ReferenceFrameKind::Perspective { scrolling_relative_to: Some(external_id) } => {
-                        // Do a change-basis operation on the perspective matrix
+                        let scroll_offset = compute_offset_from(
-                        // using the scroll offset.
+                            self.parent,
+                            external_id,
+                            previous_spatial_nodes,
+                        );
+
+                        // Do a change-basis operation on the
+                        // perspective matrix using the scroll offset.
                         source_transform
-                            .pre_translate(&state.parent_accumulated_scroll_offset)
+                            .pre_translate(&scroll_offset)
-                            .post_translate(-state.parent_accumulated_scroll_offset)
+                            .post_translate(-scroll_offset)
                     }
+                    ReferenceFrameKind::Perspective { scrolling_relative_to: None } |
                     ReferenceFrameKind::Transform => source_transform,
                 };
 

gfx/wr/webrender_api/src/display_item.rs

@@ -526,10 +526,11 @@ pub struct CacheMarkerDisplayItem {
 }
 
 #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
-#[repr(u8)]
 pub enum ReferenceFrameKind {
     Transform,
-    Perspective,
+    Perspective {
+        scrolling_relative_to: Option<ExternalScrollId>,
+    }
 }
 
 #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
@@ -949,6 +950,7 @@ impl SpatialId {
 /// When setting display lists with the `preserve_frame_state` this id is used to preserve scroll
 /// offsets between different sets of ClipScrollNodes which are ScrollFrames.
 #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+#[repr(C)]
 pub struct ExternalScrollId(pub u64, pub PipelineId);
 
 impl ExternalScrollId {

gfx/wr/wrench/src/yaml_frame_reader.rs

@@ -1675,7 +1675,7 @@ impl YamlFrameReader {
         );
 
         let reference_frame_kind = if !yaml["perspective"].is_badvalue() {
-            ReferenceFrameKind::Perspective
+            ReferenceFrameKind::Perspective { scrolling_relative_to: None }
         } else {
             ReferenceFrameKind::Transform
         };

gfx/wr/wrench/src/yaml_frame_writer.rs

@@ -196,11 +196,15 @@ fn write_reference_frame(
     properties: &SceneProperties,
     clip_id_mapper: &mut ClipIdMapper,
 ) {
+    // FIXME: This ignores the scrolling_relative_to member in
+    // ReferenceFrameKind::Perspective, but it's a bit annoying to fix since the
+    // frame reader abuses `ExternalScrollId`s.
+
     matrix4d_node(
         parent,
         match reference_frame.kind {
             ReferenceFrameKind::Transform => "transform",
-            ReferenceFrameKind::Perspective => "perspective",
+            ReferenceFrameKind::Perspective { .. } => "perspective",
         },
         &properties.resolve_layout_transform(&reference_frame.transform)
     );

layout/painting/nsDisplayList.cpp

@@ -8513,10 +8513,27 @@ bool nsDisplayPerspective::CreateWebRenderCommands(
 
   wr::StackingContextParams params;
   params.mTransformPtr = &perspectiveMatrix;
-  params.reference_frame_kind = wr::ReferenceFrameKind::Perspective;
+  params.reference_frame_kind = wr::WrReferenceFrameKind::Perspective;
   params.is_backface_visible = !BackfaceIsHidden();
   params.SetPreserve3D(preserve3D);
 
+  Maybe<uint64_t> scrollingRelativeTo;
+  for (auto* asr = GetActiveScrolledRoot(); asr; asr = asr->mParent) {
+    if (nsLayoutUtils::IsAncestorFrameCrossDoc(
+          asr->mScrollableFrame->GetScrolledFrame(), perspectiveFrame)) {
+      scrollingRelativeTo.emplace(asr->GetViewId());
+      break;
+    }
+  }
+
+  // We put the perspective reference frame wrapping the transformed frame,
+  // even though there may be arbitrarily nested scroll frames in between.
+  //
+  // We need to know how many ancestor scroll-frames are we nested in, in order
+  // for the async scrolling code in WebRender to calculate the right
+  // transformation for the perspective contents.
+  params.scrolling_relative_to = scrollingRelativeTo.ptrOr(nullptr);
+
   StackingContextHelper sc(aSc, GetActiveScrolledRoot(), mFrame, this, aBuilder,
                            params);
 

layout/reftests/async-scrolling/reftest.list

@@ -57,7 +57,7 @@ fuzzy-if(Android,0-7,0-4) skip-if(!asyncPan) == perspective-scrolling-1.html per
 fuzzy-if(Android,0-7,0-4) skip-if(!asyncPan) == perspective-scrolling-2.html perspective-scrolling-2-ref.html
 fuzzy-if(Android,0-7,0-4) skip-if(!asyncPan) == perspective-scrolling-3.html perspective-scrolling-3-ref.html
 fuzzy-if(Android,0-7,0-4) skip-if(!asyncPan) == perspective-scrolling-4.html perspective-scrolling-4-ref.html
-random-if(webrender) skip-if(!asyncPan) == perspective-scrolling-5.html perspective-scrolling-5-ref.html # bug 1498639
+skip-if(!asyncPan) == perspective-scrolling-5.html perspective-scrolling-5-ref.html
 pref(apz.disable_for_scroll_linked_effects,true) skip-if(!asyncPan) == disable-apz-for-sle-pages.html disable-apz-for-sle-pages-ref.html
 fuzzy-if(browserIsRemote&&d2d,0-1,0-22) skip-if(!asyncPan) == background-blend-mode-1.html background-blend-mode-1-ref.html
 skip-if(Android||!asyncPan) != opaque-fractional-displayport-1.html about:blank