Botond Ballo [:botond] Reporter
	Description • 10 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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 10 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 2 • 10 years ago

(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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 4 • 10 years ago

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.

	Botond Ballo [:botond] Reporter
	Comment 5 • 10 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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 7 • 10 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 10 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 9 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 10 years ago

Whoops, meant to do that in the last patch but called the wrong function. Updated the comment as well.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 10 years ago

correct patch

	Timothy Nikkel (:tnikkel)
	Comment 12 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 10 years ago

A little more cleaned up.

	Timothy Nikkel (:tnikkel)
	Comment 14 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 10 years ago

Nope not at all, I'll do that.

	Timothy Nikkel (:tnikkel)
	Comment 16 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 10 years ago

w/ pref

	Timothy Nikkel (:tnikkel)
	Comment 18 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 19 • 10 years ago

Address comment #18

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 20 • 10 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 21 • 10 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 22 • 10 years ago

(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.

	Timothy Nikkel (:tnikkel)
	Comment 23 • 10 years ago

(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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 24 • 10 years ago

With the AddClipRectToLayer changes removed. I'll handle that separately as part of bug 1100756.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 25 • 10 years ago

Same patch but with another fix: pass mIsRoot to ComputeFrameMetrics since we no longer explicitly call ComputeFrameMetrics from PaintRoot().

	Timothy Nikkel (:tnikkel)
	Comment 26 • 10 years ago

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).

	Timothy Nikkel (:tnikkel)
	Comment 28 • 10 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 29 • 10 years ago

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
}

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 30 • 10 years ago

w/ comments addressed. I don't know why the sf->IsScrollingActive was pref'd off, it doesn't seem necessary so I reverted it.

	Timothy Nikkel (:tnikkel)
	Comment 31 • 10 years ago

Comment on attachment 8537563 [details] [diff] [review]
bug1076192-containerless-scrolling.patch

Thank you.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/190df6248c74

	Carsten Book [:Tomcat]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/190df6248c74