498462 - Invalidates are not coming properly for frames with absolute position after scroll change

Status: NEW

(Core :: Layout, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Workaround, send invalidate for old scrollport and new scrollport region

We have problem with footers and heaters frames with absolute position.
After changing scroll view position, floating frames suppose to move from absolute position in old scroll-view-port (before ScrollChange), to absolute position on new scroll-view-port.... and send notify invalidate for 2 regions (previous position and new position)...

Currently there are no any invalidates from those frames, only one big invalidate for new gfxScrollableFrame rectangle
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1796

This problem not visible in normal firefox, but visible in implementations like fennec or microb-browser...

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Footer example

Comment 2

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

I'm not sure what the problem is. Are you saying you want to be able to distinguish invalidation due to scrolling from other kinds of invalidation?

Comment 3

[Oleg Romashin (:romaxa)]

footers are changing their position after changing window scroll  (window->ScrollTo)...
And there are suppose to be invalidate with 2 rectangles (old and new position).
Main problem is that we don't receive invalidate notification for old footer position.

Comment 4

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

I still don't understand. The footer DIV in your example is not changing position when you scroll.

Comment 5

[Oleg Romashin (:romaxa)]

(In reply to comment #4)
> I still don't understand. The footer DIV in your example is not changing
> position when you scroll.

Relative to scroll-view-port - no, but relative to main document 0,0 point it is changing position.

Ex:
document with size [800x2000]. scroll port [0, 0, 800x400], div[0, 50, 800x50] in global coordinates, and [0, 50, 800x50] - absolute position relative to scroll-port.

After scrollTo(0, 250), situation is so:
scroll port [0, 250, 800x400], div[0, 300, 800x50] in global coordinates, and [0, 50, 800x50] - absolute position relative to scroll-port.

Invalidate notification is coming only for [0, 250, 800x400]

I want to get invalidate notification also for previous div position in global coordinates [0, 50, 800x50]... because without it my pre-rendered surface getting dirty area, and without invalidate [0, 50, 800x50] I don't know that it is dirty.

Comment 6

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

I see. Can you rewrite your patch to use UnionRect instead of what it currently does?

Comment 7

[Oleg Romashin (:romaxa)]

(In reply to comment #6)
> I see. Can you rewrite your patch to use UnionRect instead of what it currently
> does?

I think yes... but I think it is not very correct way to fix this  problem... 

Would it be possible to get right invalidate notification after scrollTo for each layout frame which is dependent from scroll port position? Maybe in this case we can avoid dummy ScrollPort invalidate after each scroll event, and invalidate only those things which is must be repainted?

Comment 8

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

That sounds too complicated, lots of overhead and for normal use it's not necessary.

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Updated with UnionRect

Comment 10

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

Call "mWillChangePos" "mPositionBeforeScroll" and add a comment describing how it's used.

+  r.UnionRect(r, nsRect(nsPoint(mWillChangePos.x - aX, mWillChangePos.y - aY), mOuter->GetSize()));

Use nsRect(mWillChangePos - nsPoint(aX, aY), mOuter->GetSize()).

+  mWillChangePos.x = aX;
+  mWillChangePos.y = aY;

Don't do this. Instead, set them to nscoord_MIN and before you use them, assert that they've been st to something else.

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Should be ok.

Comment 12

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

+  if (mPositionBeforeScroll.x != nscoord_MIN && mPositionBeforeScroll.y != nscoord_MIN) {

This should be an assertion. They can't be nscoord_MIN, if things are working correctly.

+    mPositionBeforeScroll.x = nscoord_MIN;
+    mPositionBeforeScroll.y = nscoord_MIN;

this can be #ifdef DEBUG

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Invalidate for each frame with fixed position

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Fixed style, added check for viewPort frame

Comment 15

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

Now you're not sending any NOTIFY_ONLY invalidate when we are mIsRoot and there are no fixed frames; the first InvalidateWithFlags should be unconditional.

Comment 16

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

Comment on attachment 393998 [details] [diff] [review]
Fixed style, added check for viewPort frame

see comment #15