124554 - Scrolling of non-root HTML content does full refresh

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

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

http://lxr.mozilla.org/seamonkey/source/view/src/nsScrollPortView.cpp#496

nsScrollPortView::CannotBitBlit has a heinous bug:

> return ((trans || opacity) && !(mScrollProperties &
NS_SCROLL_PROPERTY_ALWAYS_BLIT)) || ...

"opacity" is a float. It is almost always 1.0. Therefore unless ALWAYS_BLIT is
set, we will do a full refresh on every scroll operation. Oops. This bug has
been around for over two years. nsScrollingView has the same code and bug.

Comment 1

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

Unfortunately the obvious fix for this exposes a larger problem, which is that
when a view is scrolled and it has overlapping sibling views which do not have
widgets, the pixels painted by those views will be incorrectly scrolled unless
we do something about it, and currently we don't.

In general when we scroll a view we need to look at the display list for its
bounds and see if all the elements of the display list are going to scroll along
with the view. If there are non-scrolling views (i.e. views that aren't children
of the scrolling view) we will have to do something; refreshing the entire view
will suffice. We don't need to consider areas that are covered by widgets when
we compute this display list, because the platform widget library will cut them
out of the scroll area. Thus fixed-position elements (which have widgets which
are above the main document in z-order) won't prevent a document from being
efficiently scrolled.

This general check will subsume transparency and opacity checking. If a
non-scrolled view is visible through the scrolled view, it will show up in the
display list and blitting will be disabled.

To do this right requires that we trust that widget z-ordering actually works on
every platform. Up till now the view manager has not assumed that. However,
people have assumed that in other contexts, e.g. the fix to bug 43410. We can
speed up painting and scrolling a lot if we know widget z-ordering works, so I'm
going to start using that assumption.

Comment 2

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

Attachment: Scrolling performance fix

This patch does what I said it would do :-).

When scrolling we check to see if the "after scrolling" pixels are completely
determined by views that are being scrolled. If so, then we can just scroll the
widget. If not, we have to repaint it manually.

Comment 3

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

Attachment: Better patch

This patch makes a scroll operation non-blittable if any non-blittable view
(e.g., fixed background attachment) is going to be displayed in the final
rendering. This almost never happens because fixed background attachments are
almost always on the document canvas, and CanScrollWithBitBlt is not called for
those because the blittable flags for a document canvas are checked in
nsScrollingView/nsScrollPortView.

Comment 4

[Marc Attinasi]

Comment on attachment 72403 [details] [diff] [review]
Better patch

sr=attinasi
Could you please comment in the routine nsViewManager::CanScrollWithBitBlt
about how the memory management of the display works.  It seems that
BuildDisplayLost allocates   and then you have to clean it up locally - maybe
this is common knowledge to you, but it sems interesting to me.

Comment 5

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

Attachment: Revised fix

I was adding comments to the code when I realized that I couldn't give a clear
explanation of why CanScrollBitBlt was correct. Then I realized that it
actually wasn't correct :-). I hope the new explanation makes sense; I had to
fix the code to mark aView-descendants as transparent for the optimization pass
to prevent some rather obscure problems. I also had to suppress the
AddCoveringWidgetsToOpaqueRegion pass, which I did by removing it from
BuildDisplayList entirely (and moving it to RenderViews).

I also did some more testing with Viewer Demo #9 (nested frames and IFRAMEs)
and realized that CanScrollBitBlt should just look at the scroll area after
rather than the view's whole area to avoid scrolling being unnecessarily
suppressed. Then I realized (well, I knew all along but suddenly it mattered)
that nsView::GetClippedView doesn't take account of clipping due to scrollers
(which use the nsIClipView hack). So I fixed that too, by explicitly setting
the childcliprect on nsIClipViews. (This has the nice side effect of fixing
some unnecessary repaints. If you go to Viewer Demo #9 with a trunk debug build
and turn on paint flashing, you can see that when the animated eyes are
scrolled just out of the IFRAME their area keeps getting repainted anyway.)

With this patch, all the IFRAMEs in demo #9 scroll using bitblt, and I haven't
been able to find any places where I get buggy rendering because of scrolling
when we should have repainted.

This patch will need to be reviewed again, sorry...

Comment 6

[Marc Attinasi]

Comment on attachment 74590 [details] [diff] [review]
Revised fix

The two CannotBitBlt methods sure look like they have a lot in common.	Why not
factor the actual test and just allow each variant to provide the viewManager,
scrolledViewFlags and scrollProperties?


+#define VIEW_ISSCROLLED   0x00000080

what happened to 0x00000040?


Thanks for the detailed description - the code looks nice too. sr=attinasi

Comment 7

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

> The two CannotBitBlt methods sure look like they have a lot in common.

Once we have XBL form controls checked in and on by default, we should be rid of
all uses of native scrollbars, at which point I will gladly consign
nsScrollingView to a watery grave (and there will be only one CannotBitBlt
method remaining).

> #define VIEW_ISSCROLLED   0x00000080

ISSCROLLED is private to CanScrollWithBitblt. I wanted to separate the private
flags from the used-everwhere flags but make sure the flags field still fits in
a byte.

Comment 8

[Kevin McCluskey (gone)]

Comment on attachment 74590 [details] [diff] [review]
Revised fix

r=[e-mail].

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 74590 [details] [diff] [review]
Revised fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 10

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

Checked in.