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

NEW
Unassigned

Status

()

Core
Layout
9 years ago
8 years ago

People

(Reporter: romaxa, Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 383339 [details] [diff] [review]
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...
(Reporter)

Comment 1

9 years ago
Created attachment 383341 [details]
Footer example
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?
(Reporter)

Comment 3

9 years ago
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.
I still don't understand. The footer DIV in your example is not changing position when you scroll.
(Reporter)

Comment 5

9 years ago
(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.
I see. Can you rewrite your patch to use UnionRect instead of what it currently does?
(Reporter)

Comment 7

9 years ago
(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?
That sounds too complicated, lots of overhead and for normal use it's not necessary.
(Reporter)

Comment 9

9 years ago
Created attachment 384423 [details] [diff] [review]
Updated with UnionRect
Attachment #384423 - Flags: review?(roc)
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.
(Reporter)

Comment 11

9 years ago
Created attachment 385069 [details] [diff] [review]
Should be ok.
Attachment #384423 - Attachment is obsolete: true
Attachment #385069 - Flags: review?(roc)
Attachment #384423 - Flags: review?(roc)
+  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
(Reporter)

Comment 13

9 years ago
Created attachment 393894 [details] [diff] [review]
Invalidate for each frame with fixed position
(Reporter)

Comment 14

9 years ago
Created attachment 393998 [details] [diff] [review]
Fixed style, added check for viewPort frame
Attachment #393894 - Attachment is obsolete: true
Attachment #393998 - Flags: review?(roc)
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 on attachment 393998 [details] [diff] [review]
Fixed style, added check for viewPort frame

see comment #15
Attachment #393998 - Flags: review?(roc) → review-
You need to log in before you can comment on or make changes to this bug.