Dave Townsend [:mossop] Reporter
	Description • 14 years ago

With the patch in bug 595656 landed when scrolling around in the add-ons manager you'll see bits of the border scroll along with the content. They remain there for a short time, presumably a repaint is going on after that.

Screenshot: https://bugzilla.mozilla.org/attachment.cgi?id=478953

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

A reduced testcase would probably be helpful here.

	Dave Townsend [:mossop] Reporter
	Comment 3 • 14 years ago

Pretty straightforward to reproduce, just put an iframe inside a box with border radius

	Dave Townsend [:mossop] Reporter
	Comment 4 • 14 years ago

Also see odd issues with just a scrollable div, there it occasionally paints the content outside the radius and sometimes the text inside gets clipped

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

Thanks for the testcases.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 14 years ago

We clip display items as we render them into the ThebesLayer for the scrolled content. Then when we scroll, we bump the transform on the ThebesLayer instead of repainting its contents. The problem is that the clipping here is invariant as the scroll position changes, so the rendering of the clipped content in the ThebesLayer is out of date. (This actually affects rectangular clips too, but it's less obvious because in almost all cases the area that gets scrolled into view is the only area affected, and it gets repainted anyway.)

I need to think about the cleanest way to fix this.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 7 • 14 years ago

The simplest thing to do here is to force scrolling to be "inactive" when there's an ancestor with rounded-rect clipping.

Once layers support clip-to-path (post-FF4), we should then change the way we build ThebesLayers so that for a display item D that is assigned to a layer L for its nearest active scrolled root ancestor A, clipping induced by A or its ancestors is applied directly to L instead of each display item in L. That will give us accelerated scrolling even in the presence of rounded-rect clipping ancestors.

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

Comment on attachment 493326 [details] [diff] [review]
Part 2: Ensure that a scrollframe is 'inactive' if it can't be scrolled by blitting

>@@ -1501,16 +1500,19 @@ CanScrollWithBlitting(nsIFrame* aFrame)
>+    nsIScrollableFrame* sf = do_QueryFrame(f);
>+    if (sf && nsLayoutUtils::HasNonZeroCorner(f->GetStyleBorder()->mBorderRadius))
>+      return PR_FALSE;

I meant to think about this but haven't had a chance, so I'll just ask. Why do we only need to check the scrollable frame ancestors for border radius and not all ancestors?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 14 years ago

Because border-radius only clips the children if overflow is not 'visible'.

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

Comment on attachment 493326 [details] [diff] [review]
Part 2: Ensure that a scrollframe is 'inactive' if it can't be scrolled by blitting

+++ b/layout/reftests/border-radius/scroll-1.html
+<div style="border-radius:100px; width:300px; height:300px; overflow:hidden; border:1px solid black;" id="d">
+  <div style="height:1000px;">
+  <div style="margin-top:100px; height:20px; background:blue;"></div>
+  </div>
+</div>

Can you indent the div that isn't? That confused me.

@@ -1737,16 +1754,20 @@ nsGfxScrollFrameInner::BuildDisplayList(
+  if (IsScrollingActive() && !CanScrollWithBlitting(mOuter)) {
+    MarkInactive();
+  }

Do we want to do this only when building a display list for painting?

+void nsGfxScrollFrameInner::MarkInactive()
+{
+  if (IsAlwaysActive())
+    return;
+
+  mScrollingActive = PR_FALSE;
+  mOuter->InvalidateFrameSubtree();
+}

MarkInactive is actually somewhat dangerous in that it invalidates the entire frame subtree; its name seems rather innocuous. Perhaps guard the invalidate on mScrollingActive being true?

@@ -1596,20 +1607,26 @@ void nsGfxScrollFrameInner::ScrollVisual
+  PRBool canScrollWithBlitting = CanScrollWithBlitting(mOuter);
+  if (IsScrollingActive()) {
+    if (!canScrollWithBlitting) {
+      MarkInactive();
+    } else {
+      flags |= nsIFrame::INVALIDATE_NO_THEBES_LAYERS;
+    }
+  } else if (canScrollWithBlitting) {
+    MarkActive();
   }
-  MarkActive();

Don't we want to still MarkActive if IsScrollingActive() && canScrollWithBlitting? This extends the time we keep the scroll frame active.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> +++ b/layout/reftests/border-radius/scroll-1.html
> +<div style="border-radius:100px; width:300px; height:300px; overflow:hidden;
> border:1px solid black;" id="d">
> +  <div style="height:1000px;">
> +  <div style="margin-top:100px; height:20px; background:blue;"></div>
> +  </div>
> +</div>
> 
> Can you indent the div that isn't? That confused me.

Sure.

> @@ -1737,16 +1754,20 @@ nsGfxScrollFrameInner::BuildDisplayList(
> +  if (IsScrollingActive() && !CanScrollWithBlitting(mOuter)) {
> +    MarkInactive();
> +  }
> 
> Do we want to do this only when building a display list for painting?

Yeah OK. We should check IsPaintingToWindow() actually.

> +void nsGfxScrollFrameInner::MarkInactive()
> +{
> +  if (IsAlwaysActive())
> +    return;
> +
> +  mScrollingActive = PR_FALSE;
> +  mOuter->InvalidateFrameSubtree();
> +}
> 
> MarkInactive is actually somewhat dangerous in that it invalidates the entire
> frame subtree; its name seems rather innocuous. Perhaps guard the invalidate on
> mScrollingActive being true?

Yeah OK.

> @@ -1596,20 +1607,26 @@ void nsGfxScrollFrameInner::ScrollVisual
> +  PRBool canScrollWithBlitting = CanScrollWithBlitting(mOuter);
> +  if (IsScrollingActive()) {
> +    if (!canScrollWithBlitting) {
> +      MarkInactive();
> +    } else {
> +      flags |= nsIFrame::INVALIDATE_NO_THEBES_LAYERS;
> +    }
> +  } else if (canScrollWithBlitting) {
> +    MarkActive();
>    }
> -  MarkActive();
> 
> Don't we want to still MarkActive if IsScrollingActive() &&
> canScrollWithBlitting? This extends the time we keep the scroll frame active.

Yes, good point.

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

comments addressed

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 15 • 14 years ago

This is a regression in that pages which scrolled OK before now won't.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/e870951c9167
http://hg.mozilla.org/mozilla-central/rev/2a0d0ed04874

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 14 years ago

Marked test random, since it fails on some of the edge pixels on Mac and Windows.
http://hg.mozilla.org/mozilla-central/rev/6d5bec5bfb8d

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

Should we mark is failing on windows and mac so we at least get test coverage on linux?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 14 years ago

I'm going to try to fix the test for all platforms by removing the border.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 14 years ago

http://hg.mozilla.org/try/pushloghtml?changeset=b3f754733e39 FWIW

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 14 years ago

http://hg.mozilla.org/try/rev/a112ac6b4aa3 was green on try, and can be landed.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 22 • 14 years ago

All visible issues I have seen with older builds are solved. Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110105 Firefox/4.0b9pre ID:20110105030550

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

fix test
http://hg.mozilla.org/mozilla-central/rev/b6286cc2817a