Closed Bug 602892 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mossop, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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
blocking2.0: - → ?
Duplicate of this bug: 603149
A reduced testcase would probably be helpful here.
Keywords: testcase-wanted
Attached file testcase
Pretty straightforward to reproduce, just put an iframe inside a box with border radius
Attached file 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
Thanks for the testcases.
Assignee: nobody → roc
blocking2.0: ? → betaN+
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.
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.
Whiteboard: [needs review]
Attachment #493325 - Flags: review?(tnikkel) → review+
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?
Because border-radius only clips the children if overflow is not 'visible'.
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.
(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.
Attached patch Part 2 v2Splinter Review
comments addressed
Attachment #493326 - Attachment is obsolete: true
Attachment #499780 - Flags: review?(tnikkel)
Attachment #493326 - Flags: review?(tnikkel)
Attachment #499780 - Flags: review?(tnikkel) → review+
Whiteboard: [needs review] → [needs landing]
This is a regression in that pages which scrolled OK before now won't.
Keywords: regression
http://hg.mozilla.org/mozilla-central/rev/e870951c9167
http://hg.mozilla.org/mozilla-central/rev/2a0d0ed04874
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Marked test random, since it fails on some of the edge pixels on Mac and Windows.
http://hg.mozilla.org/mozilla-central/rev/6d5bec5bfb8d
Should we mark is failing on windows and mac so we at least get test coverage on linux?
I'm going to try to fix the test for all platforms by removing the border.
http://hg.mozilla.org/try/rev/a112ac6b4aa3 was green on try, and can be landed.
Whiteboard: [needs landing]
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
Status: RESOLVED → VERIFIED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b9
Depends on: 625253
No longer depends on: 623615
Depends on: 623615
You need to log in before you can comment on or make changes to this bug.