Bug 1076192 (apz-css-trans-stage2)

Use containerless scrolling for root scroll frames, too

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: botond, Assigned: dvander)

Tracking

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 13 obsolete attachments)

18.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Multi-layer-apz (bug 967844) introduced containerless scrolling for scroll frames that are not root scroll frames.

Using containerless scrolling for root scroll frames as well would:

  1) allow us to remove a bunch of Layout code paths (the ones pertaining 
     to non-containerless ("container-y"?) scrolling)

  2) eliminate some layer trees involving CSS transforms that are 
     difficult for APZ to handle correctly, thereby solving Stage 2
     of apz-css-transforms [1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=993525#c3
(Reporter)

Updated

4 years ago
Depends on: 1076241
Created attachment 8509237 [details] [diff] [review]
WIP

This patch gets things going but there are some obvious bugs:
-- Just ripping out both calls to presShell->SetIgnoreViewportScrolling() isn't good, we need to replace them with something specific that tells the scrollframe not to draw its own scrollbars because APZ will draw them.
-- When scrolling the iframe in https://bugzilla.mozilla.org/attachment.cgi?id=8504233, we see the low-res displayport a lot. Not sure why.
-- Worse, when the scroll gesture ends, we sometimes don't rerender the low-res content at the proper resolution.
-- Sometimes the lowres content is rendered outside the subdocument clip rect, during a scroll gesture.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> -- Just ripping out both calls to presShell->SetIgnoreViewportScrolling()
> isn't good, we need to replace them with something specific that tells the
> scrollframe not to draw its own scrollbars because APZ will draw them.

We already draw the scrollbars with or without IgnoreViewportScrolling set. APZ takes what layout draws and transforms them as needed. So we shouldn't need any extra work for scrollbars here I don't think.
Created attachment 8515400 [details] [diff] [review]
containerless-scrolling.patch

Same as roc's patch, but with another root-scrollframe special case removed. With this scrolling actually works really well on win32-apzc now. I'll investigate the low-res problems roc mentioned.
Assignee: nobody → dvander
Status: NEW → ASSIGNED
I can reproduce the displayport weirdness. It looks like it has something to do with the fact that the browser itself is scrollable (its RefLayer and peer ContainerLayers are part of a single scrollID). I don't know why that causes issues yet, but I also don't know why the browser app is scrollable.
(Reporter)

Comment 5

4 years ago
(In reply to David Anderson [:dvander] from comment #4)
> I can reproduce the displayport weirdness. It looks like it has something to
> do with the fact that the browser itself is scrollable (its RefLayer and
> peer ContainerLayers are part of a single scrollID). I don't know why that
> causes issues yet, but I also don't know why the browser app is scrollable.

There is a scrollable div in the parent process which groups the rocketbar and the content iframe. This is used in conjunction with scrollgrab to implement the "dynamic rocketbar" feature, i.e. collapsing/expanding the rocketbar when you scroll up/down (see bug 835152).

This div scrolls using containerless scrolling; this is why the layers representing its content, including the RefLayer for the content iframe, get scrollable metrics.
I think the problem with the displayport is here: http://hg.mozilla.org/mozilla-central/file/d380166816dd/layout/base/nsDisplayList.cpp#l776

Before this patch, the iframe's display port got set by a special case in nsSubDocumentFrame::BuildDisplayList(), which overrode APZCCallbackHelper::UpdateSubFrame. UpdateSubFrame now gets to run instead, and it computes the display port off the composition bounds.

The code linked above computes the composition bounds relative to the scroll container. Meanwhile, ClientTiledPaintedLayer expects the bounds to be relative to its displayport ancestor. With containerless scrollingt' those can be different; in the test case, the iframe's first FrameMetrics has itself as its displayport ancestor.

Since that first FrameMetrics is computed in the wrong coordinates, tiling doesn't compute the right inversion, and ends up not including the top two tiles.
Created attachment 8519870 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Not sure if this is the right approach or not, but it does fix the low-res displayport issue on the test case in comment #1
Attachment #8509237 - Attachment is obsolete: true
Attachment #8515400 - Attachment is obsolete: true
Created attachment 8520469 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Last comment was wrong but on the right track - there was a coordinate system bug but it was in the displayport base, not the composition bounds. The displayport is supposed to be relative to the composition bounds, so the baes should start at (0, 0). Assumedly this was normally the case for root scrollframes and this only comes up for subframes, but I changed both UpdateSubFrame and UpdateRootFrame to be sure.
Attachment #8519870 - Attachment is obsolete: true
Attachment #8520469 - Flags: review?(tnikkel)
Comment on attachment 8520469 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Ah, so the bug is that we're not setting the display port base on root scroll frames anymore because GetOrMaybeCreateDisplayPort sets the display port base, and we removed that code from subdocument frame, but nsGfxScrollFrame wasn't doing it for root scroll frames.

The displayport base that APZCCallbackHelper sets should never be used in practice, it's only there as a fallback, but I think you're right that the coordinates were wrong for it.

So in nsGfxScrollFrame we should always call GetOrMaybeCreateDisplayPort.
Attachment #8520469 - Flags: review?(tnikkel)
Created attachment 8520911 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Whoops, meant to do that in the last patch but called the wrong function. Updated the comment as well.
Attachment #8520469 - Attachment is obsolete: true
Attachment #8520911 - Flags: review?(tnikkel)
Created attachment 8520912 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

correct patch
Attachment #8520911 - Attachment is obsolete: true
Attachment #8520911 - Flags: review?(tnikkel)
Attachment #8520912 - Flags: review?(tnikkel)
Comment on attachment 8520912 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Looks good, but we can do even better. We don't need an if else here because the code is now the same except for the displayPortBase.
Created attachment 8520946 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

A little more cleaned up.
Attachment #8520912 - Attachment is obsolete: true
Attachment #8520912 - Flags: review?(tnikkel)
Attachment #8520946 - Flags: review?(tnikkel)
Comment on attachment 8520946 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

In the past it's been very helpful to have an easy switch to flick to revert to the old behaviour (for debugging or QA purposes) for changes like this that are likely to lead to regressions.

Would it be too much to ask to use a hidden pref for this until we've worked through the regressions? It would involve making the SetIgnoreViewportScrolling call dependent on the pref, and making the removed code conditional on that pref.
Nope not at all, I'll do that.
And we'll also need to test this on Fennec. This is a configuration that the Fennec pan zoom controller hasn't seen before. Basic testing of a couple websites plus testing pages with fixed position content should be sufficient.
Created attachment 8522098 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

w/ pref
Attachment #8520946 - Attachment is obsolete: true
Attachment #8520946 - Flags: review?(tnikkel)
Attachment #8522098 - Flags: review?(tnikkel)
Comment on attachment 8522098 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

I think the code at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=767ad1362a2d#2921 will also need to be changed. nsDisplayList::PaintRoot will no longer attach a frame metrics, so we need to do it there instead.
Created attachment 8523692 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Address comment #18
Attachment #8522098 - Attachment is obsolete: true
Attachment #8522098 - Flags: review?(tnikkel)
Attachment #8523692 - Flags: review?(tnikkel)
Created attachment 8524867 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Removes the mAddClipRectToLayer workaround for container scroll frames.

The pref for this patch now defaults to the old behavior, so it can land before bug 1076241.
Attachment #8523692 - Attachment is obsolete: true
Attachment #8523692 - Flags: review?(tnikkel)
Attachment #8524867 - Flags: review?(tnikkel)
Comment on attachment 8524867 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

>-  mAddClipRectToLayer =
>-    !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
>+  if (gfxPrefs::LayoutUseContainersForRootFrames()) {
>+    // Root scrollframes have FrameMetrics and clipping on their container
>+    // layers, so don't apply clipping again.
>+    mAddClipRectToLayer =
>+      !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
>+  } else {
>+    mAddClipRectToLayer = true;
>+  }

I think we still need this to be the same with or without containerless scrolling.
(In reply to Timothy Nikkel (:tn) from comment #21)
> Comment on attachment 8524867 [details] [diff] [review]
> bug1076192-containerless-scrolling.patch
> 
> >-  mAddClipRectToLayer =
> >-    !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
> >+  if (gfxPrefs::LayoutUseContainersForRootFrames()) {
> >+    // Root scrollframes have FrameMetrics and clipping on their container
> >+    // layers, so don't apply clipping again.
> >+    mAddClipRectToLayer =
> >+      !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
> >+  } else {
> >+    mAddClipRectToLayer = true;
> >+  }
> 
> I think we still need this to be the same with or without containerless
> scrolling.

It's a workaround for pre-containerless-scrolling. When we're containerless we need the clip since otherwise the content layer will overlap scrollbars.
(In reply to David Anderson [:dvander] from comment #22)
> (In reply to Timothy Nikkel (:tn) from comment #21)
> > Comment on attachment 8524867 [details] [diff] [review]
> > bug1076192-containerless-scrolling.patch
> > 
> > >-  mAddClipRectToLayer =
> > >-    !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
> > >+  if (gfxPrefs::LayoutUseContainersForRootFrames()) {
> > >+    // Root scrollframes have FrameMetrics and clipping on their container
> > >+    // layers, so don't apply clipping again.
> > >+    mAddClipRectToLayer =
> > >+      !(mIsRoot && mOuter->PresContext()->PresShell()->GetIsViewportOverridden());
> > >+  } else {
> > >+    mAddClipRectToLayer = true;
> > >+  }
> > 
> > I think we still need this to be the same with or without containerless
> > scrolling.
> 
> It's a workaround for pre-containerless-scrolling. When we're containerless
> we need the clip since otherwise the content layer will overlap scrollbars.

The problem is that it is not the right clip. If you applied that clip (now or with containerless scrolling) you would sometimes be clipping out stuff that is visible on the screen.
Created attachment 8525859 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

With the AddClipRectToLayer changes removed. I'll handle that separately as part of bug 1100756.
Attachment #8524867 - Attachment is obsolete: true
Attachment #8524867 - Flags: review?(tnikkel)
Attachment #8525859 - Flags: review?(tnikkel)
No longer blocks: 918288
Created attachment 8529218 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Same patch but with another fix: pass mIsRoot to ComputeFrameMetrics since we no longer explicitly call ComputeFrameMetrics from PaintRoot().
Attachment #8525859 - Attachment is obsolete: true
Attachment #8525859 - Flags: review?(tnikkel)
Attachment #8529218 - Flags: review?(tnikkel)
Comment on attachment 8529218 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Can we split out the changes to make base rect top left at 0,0 into another bug? That is unrelated (although exposed by these changes).
Created attachment 8534675 [details] [diff] [review]
bug1076192-containerless-scrolling.patch
Attachment #8529218 - Attachment is obsolete: true
Attachment #8529218 - Flags: review?(tnikkel)
Attachment #8534675 - Flags: review?(tnikkel)
Comment on attachment 8534675 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

>diff --git a/gfx/layers/FrameMetrics.h b/gfx/layers/FrameMetrics.h
>index 3cc5fc0..5c9c6e8 100644

The comment changes to FrameMetrics.h got moved to another bug, right?

> ScrollFrameHelper::ComputeFrameMetrics(Layer* aLayer,
>   *aOutput->AppendElement() =
>       nsDisplayScrollLayer::ComputeFrameMetrics(mScrolledFrame, mOuter,
>         aContainerReferenceFrame, aLayer, mScrollParentID,
>-        scrollport, false, false, aParameters);
>+        scrollport, false, mIsRoot, aParameters);

The aIsRoot argument of ComputeFrameMetrics isn't clearly named. It means that this is the root scrollable over all documents in the tree. mIsRoot on a scroll frame indicates it is the root scroll frame of this document. So you want to also check that this is the root content document like we do in the other places when passing aIsRoot to ComputeFrameMetrics.

>@@ -448,19 +450,25 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+  if (constructResolutionItem ||
>+      constructZoomItem ||
>+      haveDisplayPort ||
>+      presContext->IsRootContentDocument() ||
>+      (gfxPrefs::LayoutUseContainersForRootFrames() && sf && sf->IsScrollingActive(aBuilder)))
>+  {
>+    needsOwnLayer = true;
>+  }

Why do we need this change to the "sf && sf->IsScrollingActive(aBuilder)" condition? AFAIK this is needed for something unrelated. Maybe it doesn't make sense where we have async pan zoom enabled though?
Comment on attachment 8534675 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

>@@ -2890,28 +2894,40 @@ ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+    if (gfxPrefs::LayoutUseContainersForRootFrames()) {
>+      if (!mIsRoot) {
>+        // For a non-root scroll frame, override the value of the display port
>+        // base rect, and possibly create a display port if there isn't one
>+        // already. For root scroll frame, nsLayoutUtils::PaintFrame or
>+        // nsSubDocumentFrame::BuildDisplayList takes care of this.
>+        nsRect displayportBase = dirtyRect;
>+        usingDisplayport = nsLayoutUtils::GetOrMaybeCreateDisplayPort(
>+            *aBuilder, mOuter, displayportBase, &displayPort);
>+      } else {
>+        // For a root frame, just get the value of the existing display port, if
>+        // any.
>+        usingDisplayport = nsLayoutUtils::GetDisplayPort(mOuter->GetContent(), &displayPort);
>+      }
>+    } else {
>+      // Override the value of the display port base rect, and possibly create a
>+      // display port if there isn't one already.
>       nsRect displayportBase = dirtyRect;
>+      if (mIsRoot && mOuter->PresContext()->IsRootContentDocument()) {
>+        displayportBase =
>+          nsRect(nsPoint(0, 0), nsLayoutUtils::CalculateCompositionSizeForFrame(mOuter));
>+      }
>       usingDisplayport = nsLayoutUtils::GetOrMaybeCreateDisplayPort(
>-          *aBuilder, mOuter, displayportBase, &displayPort);
>-    } else {
>-      // For a root frame, just get the value of the existing display port, if
>-      // any.
>-      usingDisplayport = nsLayoutUtils::GetDisplayPort(mOuter->GetContent(), &displayPort);
>+            *aBuilder, mOuter, displayportBase, &displayPort);
>     }

I think this can be more compactly written as

if (mIsRoot) {
  if (containerful) {
    // get display port
  } else { // containerless
    // get or create display port
  }
else {
  // containerful and containerless is the same for this case
}
Created attachment 8537563 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

w/ comments addressed. I don't know why the sf->IsScrollingActive was pref'd off, it doesn't seem necessary so I reverted it.
Attachment #8534675 - Attachment is obsolete: true
Attachment #8534675 - Flags: review?(tnikkel)
Attachment #8537563 - Flags: review?(tnikkel)
Comment on attachment 8537563 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Thank you.
Attachment #8537563 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/190df6248c74
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
No longer blocks: 1109949
Depends on: 1109949
No longer blocks: 1102427
You need to log in before you can comment on or make changes to this bug.