602892 - When a scrollable box appears inside a box with border-radius and that border overlaps the scrollable area scrolling causes parts of the border to bleed into the scrolled content

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Dave Townsend [:mossop]]

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

Comment 2

[Timothy Nikkel (:tnikkel)]

A reduced testcase would probably be helpful here.

Comment 3

[Dave Townsend [:mossop]]

Attachment: testcase

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

Comment 4

[Dave Townsend [:mossop]]

Attachment: testcase 2

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

Comment 5

[Timothy Nikkel (:tnikkel)]

Thanks for the testcases.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Refactor so that mScrollingActive is always true for scrollframes that are always active

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Ensure that a scrollframe is 'inactive' if it can't be scrolled by blitting

Comment 10

[Timothy Nikkel (:tnikkel)]

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

comments addressed

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 18

[Timothy Nikkel (:tnikkel)]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 23

[Timothy Nikkel (:tnikkel)]

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