Closed Bug 593243 Opened 14 years ago Closed 13 years ago

Content invalidations outside the root HTML scroll frame but inside the displayport are ignored

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: cjones, Assigned: cjones)

References

()

Details

(Keywords: polish, Whiteboard: [has patch])

Attachments

(2 files, 5 obsolete files)

Load the URL above and fire up test-ipcbrowser.  Set the CSS viewport to 600x400 and the displayport 600x600.  The red box is supposed to move left->right smoothly between the green and yellow ones, but in test-ipcbrowser it doesn't.

The invalidate is currently being clipped to the scroll frame bounds at nsHTMLScrollFrame::InvalidateInternal.  After removing this clip, something up the chain is still clipping it out.  The fix for this looks to be somewhat involved, so spinning it off of bug 590294.
Is anyone seeing this in day-to-day browsing?  If not, I'd recomment blocking-betaN.
tracking-fennec: --- → ?
aakash, can you verify that this is a problem for you on devices?
I don't think one could reproduce this on device with the URL above.  A version of that test modified to be much taller, with the animated box below the 1000th pixel, should work.
Assignee: nobody → jones.chris.g
tracking-fennec: ? → 2.0+
My understanding is that this is really hard to encounter in real world browsing, so we won't block fennec on it. If my understanding is incorrect please re-request blocking and explain.
tracking-fennec: 2.0+ → 2.0-
Attached patch "Proof-of-concept" (obsolete) — Splinter Review
I always end up having to relearn this code every other month or so when I get back to this bug.  Here's the effect that needs to happen to pass the attached test (though of course substituting clipping to infinity for clipping to the displayport).
Comment on attachment 515287 [details] [diff] [review]
"Proof-of-concept"

I'm not sure why you need the view manager changes.
Hey Timothy, I pinged you yesterday to ask how high on your todo list is removing view/.  If it's in the foreseeable future, I'd rather hold off on this bug until after views are gone.  (Unless this bug causes serious issues in the meantime.)

(In reply to comment #8)
> I'm not sure why you need the view manager changes.

The root view for content's presshell is first sized to its puppetwidget dimensions and then later resized to the CSS viewport dimensions if the viewport is overridden.  The view manager uses those dimensions to clip invalidation area.  In both those cases, when there's a displayport set that's different than the widget or viewport bounds (common case in fennec), then the view manager needs to clip to the displayport instead.
You're going to have to make nice with view/ for a while yet no matter how high on my priority list it is.

And then the puppet widget's size never changes? It sort of sucks that we need logic to handle parts of widgets "getting clipped out".
I spent an unusual amount of time on amazon.com this weekend and happened to notice a little spinner animation and a box at the bottom of product pages on the full site in which I'm pretty sure this bug is biting.  Still don't think we'd block for that, so not re-requesting triage, but I'd like to fix this.

(In reply to comment #10)
> And then the puppet widget's size never changes? It sort of sucks that we need
> logic to handle parts of widgets "getting clipped out".

Not sure I follow.  The displayport and widget sizes can change independently because displayport is explicitly designed to be a way to make more content visible without reflowing, which widget resizes require.  The bug here bites when the displayport is larger than the the root view for the content document.  Note that the root view and widget sizes can change independently too, because the view bounds are updated on reflows, and those can be forced by setting a custom CSS viewport of a different size than the widget.  When the root view is smaller than the displayport, and content outside the view but inside the displayport is invalidated, the view/ code doesn't honor the invalidation because it gets intersected with view bounds.
Keywords: polish
Blocks: 639406
Requesting blocking since this blocks a blocker. Bug 639406 is in fact an easy way to reproduce the problem here.
tracking-fennec: 2.0- → ?
tracking-fennec: ? → 2.0+
Patching code way out of my comfort zone, please be brutal.
Attachment #515287 - Attachment is obsolete: true
Attachment #517996 - Flags: superreview?(roc)
Attachment #517996 - Flags: review?(tnikkel)
Comment on attachment 517996 [details] [diff] [review]
Clip invalidations to the displayport when one is set

@@ -190,17 +190,25 @@ nsHTMLScrollFrame::InvalidateInternal(co
-      parentDamage.IntersectRect(damage, mInner.mScrollPort);
+      nsIPresShell* presShell = PresContext()->PresShell();
+      // If we're using a displayport, we might be displaying an area
+      // different than our scroll port and the damage needs to be
+      // clipped to that instead.
+      if (presShell->UsingDisplayPort()) {
+        parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
+      } else {
+        parentDamage.IntersectRect(damage, mInner.mScrollPort);
+      }

You probably only want to do this if mIsRoot is true. You'll probably also want to make the same changes to nsXULScrollFrame::InvalidateInternal

diff --git a/view/public/nsIView.h b/view/public/nsIView.h
+  void SetInvalidationBounds(const nsRect* aRect);

IID bump! (sadly)

Add a comment to SetInvalidationBounds and mInvalidationBounds to indicate what they are relative to (either parent view or |this|, see below).

+void nsView::SetInvalidationBounds(const nsRect* aRect)
+{
+  mHaveInvalidationBounds = !!aRect;
+  if (mHaveInvalidationBounds) {
+    nsRect bounds = *aRect;
+    bounds.MoveBy(mPosX, mPosY);
+    mInvalidationBounds = bounds;
+    // There's nothing more we can do here ... if we have pending
+    // invalidations already clipped to the old bounds, it's not
+    // possible to restore them.  And if the bounds shrunk, then when
+    // we get around to painting, we'll clip properly.
+  }
+}

So bounds.MoveBy(mPosX, mPosY); takes bounds (which are relative to |this|) and converts them to be relative to the parent view? I don't think that's what you want as GetInvalidationBounds is used in the same manner as GetDimensions, and GetDimensions is relative to |this|. So perhaps SetInvalidationBounds should be called SetInvalidationDimensions for symmetry as GetBounds is relative to the parent view.
(In reply to comment #14)
> Comment on attachment 517996 [details] [diff] [review]
> Clip invalidations to the displayport when one is set
> 
> @@ -190,17 +190,25 @@ nsHTMLScrollFrame::InvalidateInternal(co
> -      parentDamage.IntersectRect(damage, mInner.mScrollPort);
> +      nsIPresShell* presShell = PresContext()->PresShell();
> +      // If we're using a displayport, we might be displaying an area
> +      // different than our scroll port and the damage needs to be
> +      // clipped to that instead.
> +      if (presShell->UsingDisplayPort()) {
> +        parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
> +      } else {
> +        parentDamage.IntersectRect(damage, mInner.mScrollPort);
> +      }
> 
> You probably only want to do this if mIsRoot is true. You'll probably also want
> to make the same changes to nsXULScrollFrame::InvalidateInternal

OK.

> diff --git a/view/public/nsIView.h b/view/public/nsIView.h
> +  void SetInvalidationBounds(const nsRect* aRect);
> 
> IID bump! (sadly)

For changing the class instance layout, you sure?  I guess I could just move the *InvalidationBounds member vars into nsView.

> +void nsView::SetInvalidationBounds(const nsRect* aRect)
> +{
> +  mHaveInvalidationBounds = !!aRect;
> +  if (mHaveInvalidationBounds) {
> +    nsRect bounds = *aRect;
> +    bounds.MoveBy(mPosX, mPosY);
> +    mInvalidationBounds = bounds;
> +    // There's nothing more we can do here ... if we have pending
> +    // invalidations already clipped to the old bounds, it's not
> +    // possible to restore them.  And if the bounds shrunk, then when
> +    // we get around to painting, we'll clip properly.
> +  }
> +}
> 
> So bounds.MoveBy(mPosX, mPosY); takes bounds (which are relative to |this|) and
> converts them to be relative to the parent view? I don't think that's what you
> want as GetInvalidationBounds is used in the same manner as GetDimensions, and
> GetDimensions is relative to |this|. So perhaps SetInvalidationBounds should be
> called SetInvalidationDimensions for symmetry as GetBounds is relative to the
> parent view.

Thanks, this was the biggest thing I was unsure about.  Your suggestion sounds good.
(In reply to comment #15)
> For changing the class instance layout, you sure?  I guess I could just move
> the *InvalidationBounds member vars into nsView.

Oh, ok, maybe not then.
Attachment #517996 - Attachment is obsolete: true
Attachment #518110 - Flags: superreview?(roc)
Attachment #518110 - Flags: review?(tnikkel)
Attachment #517996 - Flags: superreview?(roc)
Attachment #517996 - Flags: review?(tnikkel)
(In reply to comment #14)
> Add a comment to SetInvalidationBounds and mInvalidationBounds to indicate what
> they are relative to (either parent view or |this|, see below).

Grr sorry, forgot this.  Sec.
Attachment #518110 - Attachment is obsolete: true
Attachment #518113 - Flags: superreview?(roc)
Attachment #518113 - Flags: review?(tnikkel)
Attachment #518110 - Flags: superreview?(roc)
Attachment #518110 - Flags: review?(tnikkel)
+    // There's nothing more we can do here ... if we have pending
+    // invalidations already clipped to the old bounds, it's not
+    // possible to restore them.

Won't the difference in areas be invalidated anyway? If not, it should be!

+  PRBool       mHaveInvalidationDimBounds;

PRPackedBool
(In reply to comment #20)
> +    // There's nothing more we can do here ... if we have pending
> +    // invalidations already clipped to the old bounds, it's not
> +    // possible to restore them.
> 
> Won't the difference in areas be invalidated anyway? If not, it should be!

Yes, the area is invalidated in presshell just after calling view->SetInvalidationDimensions().  Should view do something too?

> +  PRBool       mHaveInvalidationDimBounds;
> 
> PRPackedBool

Sure.
Comment on attachment 518113 [details] [diff] [review]
Clip invalidations to the displayport when one is set, v3

+  nsRect GetInvalidationBounds() const {
+    nsRect r = mHaveInvalidationDimBounds ? mInvalidationDimBounds : mDimBounds;
+    r.MoveBy(-mPosX, -mPosY);
+    return r;
+  }

Wait, I thought the rect we pass SetInvalidationBounds was relative to the root view already?
(In reply to comment #21)
> Yes, the area is invalidated in presshell just after calling
> view->SetInvalidationDimensions().  Should view do something too?

No, but you should change the comment to indicate that the the caller is responsible for invalidating the difference.
(In reply to comment #22)
> Comment on attachment 518113 [details] [diff] [review]
> Clip invalidations to the displayport when one is set, v3
> 
> +  nsRect GetInvalidationBounds() const {
> +    nsRect r = mHaveInvalidationDimBounds ? mInvalidationDimBounds :
> mDimBounds;
> +    r.MoveBy(-mPosX, -mPosY);
> +    return r;
> +  }
> 
> Wait, I thought the rect we pass SetInvalidationBounds was relative to the root
> view already?

I think so, because we only happen to use this for root views at the moment.  But I was trying to address comment 14 with this; did I do it wrong?  (Still not clear in my mind.)

(In reply to comment #23)
> No, but you should change the comment to indicate that the the caller is
> responsible for invalidating the difference.

Sure.
(In reply to comment #24)
> > Wait, I thought the rect we pass SetInvalidationBounds was relative to the root
> > view already?
> 
> I think so, because we only happen to use this for root views at the moment. 
> But I was trying to address comment 14 with this; did I do it wrong?  (Still
> not clear in my mind.)

Here is what I think. The rect we pass to SetInvalidationDimensions is relative to the root frame/view. GetDimensions is relative to the view. GetInvalidationBounds/Dimensions is used in the same manner as GetDimensions. So GetInvalidationBounds/Dimensions should be relative to the view. So we shouldn't need to change the rect passed to SetInvalidationDimensions at all. Does this jive with your understanding?
Yes, but in my limited understanding, we can get away with that only because we're calling SetInvalidationBounds() on just root views atm, which should also have mPosX/Y = 0 so the MoveBy() is a no-op.  (I think?)

I'm happy assuming that and not doing the MoveBy() on mInvalidationDimBounds, or not assuming that and continuing to do the no-op MoveBy() in case we ever use custom invalidation-bounds on non-root views.  Whichever the view peers prefer :).
You are correct that mPosX/Y = 0 on root views, but it is my belief that we don't need the MoveBy even for non-root views. But I guess this all depends on the rect that is passed to SetInvalidationBounds for them.
I added a test that simulates text input outside the root scroll frame by setting .value on an <input>, and that test fails before this patch and passes with it :/.

I'll see if I can figure out what's up with bugzilla's input, and if not, go with bug 639406 comment 20.
Have an updated patch ready, will post after incorporating upcoming comments from Timothy.
Comment on attachment 518113 [details] [diff] [review]
Clip invalidations to the displayport when one is set, v3

+  nsIView* rootView;
+  if (NS_SUCCEEDED(mViewManager->GetRootView(rootView)) && rootView) {
+    rootView->SetInvalidationDimensions(&mDisplayPort);
+  }
+
   // FIXME (Bug 593243 should fix this.)

Hey, wait a minute, this is bug 593243. So does it fix that? We gonna leave this open or file a new bug if it doesn't?

diff --git a/view/public/nsIView.h b/view/public/nsIView.h
+  void SetInvalidationDimensions(const nsRect* aRect);

Add a comment saying what coordinate system the rect is in. In this case I think it should be relative to |this|.

@@ -1099,17 +1107,25 @@ void
 nsXULScrollFrame::InvalidateInternal(const nsRect& aDamageRect,
+    nsIPresShell* presShell = PresContext()->PresShell();
+    // If we're using a displayport, we might be displaying an area
+    // different than our scroll port and the damage needs to be
+    // clipped to that instead.
+    if (mInner.mIsRoot && presShell->UsingDisplayPort()) {
+      parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
+    } else {
+      parentDamage.IntersectRect(damage, mInner.mScrollPort);
+    }

I realized after you made the two changes that I asked for that XUL scroll frames can never be root scroll frames. When other non-root scroll frames get displayports this would be important though right?

+  nsRect GetInvalidationBounds() const {
+  nsRect       mInvalidationDimBounds;
+  PRBool       mHaveInvalidationDimBounds;

Let's always call it InvalidationDimensions (mDimBounds is relative to the parent view).

+  // invalidations are clipped to mInvalidationDimBounds, not
+  // mDimBounds, when mHaveInvalidationDimBounds is true.  This is
+  // used to support persistent "displayport" rendering; see
+  // nsPresShell.cpp.  Like mDimBounds, the coordinates of
+  // mInvalidationDimBounds are relative to |this|.
+  nsRect       mInvalidationDimBounds;
+  PRBool       mHaveInvalidationDimBounds;

I thought mDimBounds was relative to the parent view. Was there something that made you think otherwise? We should fix that then.
(In reply to comment #30)
> Comment on attachment 518113 [details] [diff] [review]
> Clip invalidations to the displayport when one is set, v3
> 
> +  nsIView* rootView;
> +  if (NS_SUCCEEDED(mViewManager->GetRootView(rootView)) && rootView) {
> +    rootView->SetInvalidationDimensions(&mDisplayPort);
> +  }
> +
>    // FIXME (Bug 593243 should fix this.)
> 
> Hey, wait a minute, this is bug 593243. So does it fix that? We gonna leave
> this open or file a new bug if it doesn't?
> 

Hmmmm.  I think this might have been trying to work around subframes not being repainted after having a displayport set.  Shouldn't really make a difference now, perf-wise, since the INVALIDATE_NO_THEBES_LAYER bug was fixed, and invalidating the root is certainly simpler.  I'll just drop the comment.

> diff --git a/view/public/nsIView.h b/view/public/nsIView.h
> +  void SetInvalidationDimensions(const nsRect* aRect);
> 
> Add a comment saying what coordinate system the rect is in. In this case I
> think it should be relative to |this|.
> 

Sure.

> @@ -1099,17 +1107,25 @@ void
>  nsXULScrollFrame::InvalidateInternal(const nsRect& aDamageRect,
> +    nsIPresShell* presShell = PresContext()->PresShell();
> +    // If we're using a displayport, we might be displaying an area
> +    // different than our scroll port and the damage needs to be
> +    // clipped to that instead.
> +    if (mInner.mIsRoot && presShell->UsingDisplayPort()) {
> +      parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
> +    } else {
> +      parentDamage.IntersectRect(damage, mInner.mScrollPort);
> +    }
> 
> I realized after you made the two changes that I asked for that XUL scroll
> frames can never be root scroll frames. When other non-root scroll frames get
> displayports this would be important though right?

Oh actually I think they already do.  Dropped the check for root.

> 
> +  nsRect GetInvalidationBounds() const {
> +  nsRect       mInvalidationDimBounds;
> +  PRBool       mHaveInvalidationDimBounds;
> 
> Let's always call it InvalidationDimensions (mDimBounds is relative to the
> parent view).

OK.

> +  // invalidations are clipped to mInvalidationDimBounds, not
> +  // mDimBounds, when mHaveInvalidationDimBounds is true.  This is
> +  // used to support persistent "displayport" rendering; see
> +  // nsPresShell.cpp.  Like mDimBounds, the coordinates of
> +  // mInvalidationDimBounds are relative to |this|.
> +  nsRect       mInvalidationDimBounds;
> +  PRBool       mHaveInvalidationDimBounds;
> 
> I thought mDimBounds was relative to the parent view. Was there something that
> made you think otherwise? We should fix that then.

No, I think I was just confused.
I'm not sure if this works correctly for non-root views (I /think/ it does), but I don't think we really care about that.
Attachment #518113 - Attachment is obsolete: true
Attachment #518671 - Flags: review?(tnikkel)
Attachment #518113 - Flags: superreview?(roc)
Attachment #518113 - Flags: review?(tnikkel)
Comment on attachment 518671 [details] [diff] [review]
Clip invalidations to the displayport when one is set

+      nsIPresShell* presShell = PresContext()->PresShell();
+      // If we're using a displayport, we might be displaying an area
+      // different than our scroll port and the damage needs to be
+      // clipped to that instead.
+      if (presShell->UsingDisplayPort()) {
+        parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
+      } else {
+        parentDamage.IntersectRect(damage, mInner.mScrollPort);
+      }

Until bug 618975 lands then I think only root scroll frames can have a display port. presShell->GetDisplayPort() is relative to the root frame. damage is relative to this scroll frame, and if it isn't the root scroll frame then this is wrong. I think we still need the mIsRoot check to be correct here.

   /**
+   * Set the bounds at which invalidations are clipped, which can be
+   * different than |GetBounds()|.  |aRect| is relative to |this|.  It
+   * can be null, in which case invalidations return to being clipped
+   * to the bounds.
+   *
+   * The caller is responsible for invalidating the area that may lie
+   * outside the bounds but inside |aRect| after this call.
+   */
+  void SetInvalidationDimensions(const nsRect* aRect);

Set the _dimensions_ ... which can be different than GetDimensions ... clipped to the dimensions.

+  // invalidations are clipped to mInvalidationDimBounds, not
+  // GetDimensions(), when mHaveInvalidationDimBounds is true.  This
+  // is used to support persistent "displayport" rendering; see
+  // nsPresShell.cpp.  The coordinates of mInvalidationDimensions are
+  // relative to the root view.
+  nsRect       mInvalidationDimensions;
+  PRPackedBool mHaveInvalidationDimensions;

mInvalidationDimBounds, mHaveInvalidationDimBounds in the comments needs updating. Just say the coordinates of mInvalidationDimensions are relative to |this|, because they are.
(In reply to comment #33)
> Comment on attachment 518671 [details] [diff] [review]
> Clip invalidations to the displayport when one is set
> 
> +      nsIPresShell* presShell = PresContext()->PresShell();
> +      // If we're using a displayport, we might be displaying an area
> +      // different than our scroll port and the damage needs to be
> +      // clipped to that instead.
> +      if (presShell->UsingDisplayPort()) {
> +        parentDamage.IntersectRect(damage, presShell->GetDisplayPort());
> +      } else {
> +        parentDamage.IntersectRect(damage, mInner.mScrollPort);
> +      }
> 
> Until bug 618975 lands then I think only root scroll frames can have a display
> port. presShell->GetDisplayPort() is relative to the root frame. damage is
> relative to this scroll frame, and if it isn't the root scroll frame then this
> is wrong. I think we still need the mIsRoot check to be correct here.
> 

Fixed.

>    /**
> +   * Set the bounds at which invalidations are clipped, which can be
> +   * different than |GetBounds()|.  |aRect| is relative to |this|.  It
> +   * can be null, in which case invalidations return to being clipped
> +   * to the bounds.
> +   *
> +   * The caller is responsible for invalidating the area that may lie
> +   * outside the bounds but inside |aRect| after this call.
> +   */
> +  void SetInvalidationDimensions(const nsRect* aRect);
> 
> Set the _dimensions_ ... which can be different than GetDimensions ... clipped
> to the dimensions.

Heh, done.

> +  // invalidations are clipped to mInvalidationDimBounds, not
> +  // GetDimensions(), when mHaveInvalidationDimBounds is true.  This
> +  // is used to support persistent "displayport" rendering; see
> +  // nsPresShell.cpp.  The coordinates of mInvalidationDimensions are
> +  // relative to the root view.
> +  nsRect       mInvalidationDimensions;
> +  PRPackedBool mHaveInvalidationDimensions;
> 
> mInvalidationDimBounds, mHaveInvalidationDimBounds in the comments needs
> updating. Just say the coordinates of mInvalidationDimensions are relative to
> |this|, because they are.

Yeah sorry, I fixed this locally just after posting.  Done.
Attachment #518671 - Attachment is obsolete: true
Attachment #518758 - Flags: review?(tnikkel)
Attachment #518671 - Flags: review?(tnikkel)
Whiteboard: [has patch][needs review]
Attachment #518758 - Flags: review?(tnikkel) → review+
Whiteboard: [has patch][needs review] → [has patch]
Small, but possibly real, regression on Tsvg reported by the automated mails
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/9fa35b0275f2054d#
And then it went down a week later for no reason.
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/3efb2cebc40be5bb#
So probably nothing happened here.
You need to log in before you can comment on or make changes to this bug.