BeOS Scrolling needs to be rewritten

RESOLVED FIXED

Status

SeaMonkey
UI Design
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Daniel Switkin, Assigned: Sergei Dolgov)

Tracking

({fixed1.8})

Trunk
Other
BeOS
fixed1.8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

16 years ago
The biggest problem here is that children views do not scroll in sync with the parent, 
which causes streaks of garbage pixels while scrolling (which eventually get repainted). 
You can see this on any page which has iframes. The solution will needs to keep them in 
sync while retaining the hardware screen-to-screen blit for performance. Early experiments 
have not been that promising - it's a tough problem.
(Assignee)

Comment 1

16 years ago
I tried to put existing NS_METHOD nsWindow::Scroll() code with all those blits
into children iteration loop, directly replaced in loop  mView with its children.
All seem stay in sync in this case, but scrolls very slowly.
(Assignee)

Comment 2

16 years ago
I'm nor web-design guru, neither mozilla code specialist, but i wonder if BViews' nesting degree in Mozilla rendered page may be more than 1.
In such case even this "buggy" scrolling  may appear non-working.

Comment 3

15 years ago
confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

15 years ago
Code in mozilla/view/*
"moves" main view (and children) without repainting by calling SetPosition().
Then checks if widget can BitBlt but not for children.
If cannot BitBlt, it invalidates main view and "adjusts" children by itself.
BeOS case is a bit messy - coz it gets CanBitBlt property, it don't adjust
children, so we should move already "moved" child views. Last fact can be
checked if comment out children moving in BeOS widget Scroll () code, scroll
some page with inline frames and then resize window - all "unscrolled" children
jump on proper position.

Comment 5

15 years ago
By separating the Scroll methods into ScrollWidgets and ScrollRect, we should be
able to fix the scrolling of child views.  We could, theorectially, lock the
looper on the parent, scroll the children, then self, and then, unlock the looper.

Currently the BaseWidget contains an empty implementation for the ScrollWidgets
and ScrollRect.  If am not sure if they are actually being called, yet.
Depends on: 24240
(Assignee)

Comment 6

15 years ago
I tried to create both, but those seem never called.
(Assignee)

Comment 7

15 years ago
btw, i think that origin of problem is that child widgets are also BeOS BView
children in current implementation. Not sure if we really need that, maybe it is
possible to add those to parent's BWindow() as children. And Mozilla should care
about other aspects of parent/child/sibling relations itself.
(Reporter)

Comment 8

15 years ago
Right Sergei. Since they are child BViews (which may or may not be necessary, I don't know) 
they scroll in the BeOS fashion, which is to change the topleft coordinate of the parent. 
So when scrolling down 10 pixels, the parent corner is 0, 10 and all the children stay 
where they are. The Mozilla model as I understand it is to keep the parent corner always at 
0, 0 and then move the chidren. Therefore we can't just use ScrollTo or ScrollBy in BView 
(too bad, because they're hardware accelerated). Right now, you see garbage because 
CopyBits for the parent is very fast, but moving and redrawing all the children is quite 
slow (it lags behind). Unfortunately there is no way to tell CopyBits to move the 
childrens' data also (that's what ScrollTo is for). So other than doing scrolling entirely 
in software (I tried, it's unusably slow) there are two paths I can think of:

1. Use ScrollTo and ScrollBy but then compensate for the offset (see SetOrigin() and 
Origin() in BView.h) whenever drawing or dealing with mouse coordinates.
2. Try to implement IFRAMES and whatever else uses child BViews right now in a different 
way where you have one big view, and then use CopyBits for the scrolling.

I'm no longer coding on Mozilla, so feel free to reassign the bug. But I'm happy to discuss 
ways of fixing this and maybe reviewing the code.
(Assignee)

Comment 9

15 years ago
reminder for myself - should look at http://www.3rd-evolution.de/BeOS/MDI/
(Assignee)

Comment 10

15 years ago
Daniel, i found wasy how to do bit copy for all page content, only (but seems
serious) problem are child ghosts which coexist with scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.Daniel, i found wasy how to do bit copy for all page
content, only (but seems serious) problem are child ghosts which coexist with
scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.Daniel, i found wasy how to do bit copy for all page
content, only (but seems serious) problem are child ghosts which coexist with
scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.
(Assignee)

Comment 11

14 years ago
After thousands of experiments i found only way to reduce i-frame lagging.
It is:
override BView::DrawAfterChildren;
in ::Scroll, while moving children, apply B_DRAW_ON_CHILDREN flag.
reset that flag after children were moved.

Unfortunately those children, while don't lagging anymore, blink a  bit.
Especially when child's frame reaches parent frame border.
Product: Browser → Seamonkey
(Assignee)

Comment 12

13 years ago
*** Bug 275380 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Component: General → XP Apps: GUI Features

Comment 13

12 years ago
(In reply to comment #11)
> After thousands of experiments i found only way to reduce i-frame lagging.
> It is:
> override BView::DrawAfterChildren;
> in ::Scroll, while moving children, apply B_DRAW_ON_CHILDREN flag.
> reset that flag after children were moved.
> 
> Unfortunately those children, while don't lagging anymore, blink a  bit.
> Especially when child's frame reaches parent frame border.

Sergei, seems like this may have been taken care of as part of another
bug/patch.  Is this still a valid bug to have open? 

Comment 14

12 years ago
The blinking is still there.

Comment 15

12 years ago
(In reply to comment #14)
> The blinking is still there.

True, it seems to be.  But doesn't that mean the code discussed here is already
implemented in some other patch?  Otherwise, we'd still have the streaks of
garbage pixels discussed in the original report.  So the original problem seems
fixed and not because of a patch for this bug but for some other.
(Assignee)

Comment 16

12 years ago
This bug is still valid.
Both description and origin of problem.
All measures we took were palliatives, so atm, we still have to choose between
two evils - ugly lagging and irritating blinking.
(Assignee)

Comment 17

12 years ago
Created attachment 197343 [details] [diff] [review]
Scroll and Paint rewrite

Patch. 

Scrolling changes:
Hiding children widgets while scrolling, unhiding at ::Update().
Calculating in-screen rect.
Painting dirty area at once, instead traversing rects - will be rewritten when
NS_PAINT really process regions, instead taking frame of those.

Painting changes:
1)nsViewBeOS::GetPaintingRegion() don't empty now whole pending drawing region,
there is new method for that nsViewBeOS::Validate(BRect), we are calling it in
::OnPaint(). So this substraction affect both synchronous and asynchronous
drawing.
2)::Ivalidate() group of methods now calls again BView::Draw() instead
Invalidate() in case of asynchronous painting. As we calculate update rects
correctly now in ::Scroll() - no more app_server involvement required.
3)Only eWindowType_child and _popup are drawing now. Includes changes in
::OnPaint() method, in ::StandardWindowCreate() and in ::CreateBeOSView()

I wish someone test this on plain cvs build, as we (me and tqh) are tested that

 with very non-standard sources only.
Attachment #197343 - Flags: review?(thesuckiestemail)

Comment 18

12 years ago
I think we should reduce code in OnPaint by removing 
	static NS_DEFINE_IID(kRenderingContextCID, NS_RENDERING_CONTEXT_CID);
	static NS_DEFINE_IID(kRenderingContextIID, NS_IRENDERING_CONTEXT_IID);

	if (NS_SUCCEEDED(CallCreateInstance(kRenderingContextCID,
&event.renderingContext)))
	{
		event.renderingContext->Init(mContext, this);
		result = DispatchWindowEvent(&event);

		NS_RELEASE(event.renderingContext);
		result = PR_TRUE;
	}
	else
		result = PR_FALSE;

and replacing it with
	event.renderingContext = GetRenderingContext();
	if(event.renderingContext != nsnull)
	{
		result = DispatchWindowEvent(&event);
		NS_RELEASE(event.renderingContext);
	} else {
		result = PR_FALSE;
	}
(Assignee)

Comment 19

12 years ago
tqh, you didn't answer to my notice before about
event.renderingContext = GetRenderingContext();
maybe you missed it.

Ias far as i investigated it, GetRenderingContext in our case did absolutely same
as we do, aonly adding call overhead. Maybe it was changed, but at least you can
point me at current GetRenderingContext implementation

Comment 20

12 years ago
patch applies cleanly to CVS but breaks build.  Also verified middle-button
scrolling icon leaves a trail.
(Assignee)

Comment 21

12 years ago
Doug, what is version you used to patch?
CVS may provide various versions.
Post full info from About page
(Assignee)

Comment 22

12 years ago
Comment on attachment 197343 [details] [diff] [review]
Scroll and Paint rewrite

found bug in Validate() declaration
Attachment #197343 - Attachment is obsolete: true
Attachment #197343 - Flags: review?(thesuckiestemail)
(Assignee)

Comment 23

12 years ago
Created attachment 197354 [details] [diff] [review]
patch

Same as previous but :Validate() implementation now follows declaration.
Also unused variable in OnPaint removed.
Waiting also for tqh's comment about GetRenderingContext() benefits:
http://lxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsBaseWidget.cpp#605

http://lxr.mozilla.org/seamonkey/source/gfx/src/nsDeviceContext.cpp#220 (we
lack proprietary implemetation atm)
do_CreateInstance(kRenderingContextCID, &rv); - couldn't find code for that,
only
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsGenericFactory.cpp#75

Updated

12 years ago
QA Contact: doronr → nobody

Comment 24

12 years ago
second patch version applies cleanly to CVS 
Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20050925 Firefox/1.6a1

fyysik, I hope this answers question about the version.  

Comment 25

12 years ago
second patch version scrolls without flicker on pages with iFrames. 
Center-button scroll mouse trails are a separate problem - happen with CVS
version and also after patching.

Comment 26

12 years ago
Fyysik, the only reason for getRenderingContext is so that we don't increase
code-size with duplicate functionality and if something is changed in
GetRenderingContext we don't have to do the same changes.

Btw, the overhead is unnoticable for me.
(Assignee)

Comment 27

12 years ago
Created attachment 197417 [details] [diff] [review]
patch

Same as above but:
1)sibling clipping check in ::Scroll() added
2)nsViewBeOS::DrawAferChildren() removed
3)Begin/EndResizingChildren emptied (to implement)
4)Added nsIRegion painting (mUpdateArea in *.h) to Scroll(), OnPaint(),
InvalidateRegion(), CallMethod()ONPAINT:
5)GetRenderingContext() used in OnPaint()

Hope tigerdog and tqh's will test it soon.
Attachment #197417 - Flags: review?(thesuckiestemail)

Comment 28

12 years ago
downloaded nsWindow from CVS, applied 3rd version of patch.  anomalies with
chatzilla and 3rd button mouse-scroll icon are gone.  iFrame scrolling is much
cleaner than CVS version.  Scrolling is much improved over current CVS.

Comment 29

12 years ago
Comment on attachment 197417 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se

Tested it and looked at the code, much better than before.
Attachment #197417 - Flags: review?(thesuckiestemail) → review+

Comment 30

12 years ago
It would be good if we could add this to the 1.8 branch as well. So that it is
included in Firefox 1.5 release.

Updated

12 years ago
Blocks: 266252
(Assignee)

Updated

12 years ago
Attachment #197354 - Attachment is obsolete: true
(Assignee)

Comment 31

12 years ago
patch commited into trunk:
nsWindow.cpp
new revision: 1.95; previous revision: 1.94 
nsWindow.h
new revision: 1.36; previous revision: 1.35

Nielx, can you take care about branch?
Assignee: mozilla → sergei_d

Comment 32

12 years ago
Comment on attachment 197417 [details] [diff] [review]
patch

This patch touches BeOS-only files and therefore shouldn't be influencing the
quality of any other platform. Patch revieuwed, working verified and approval
will be highly appreciated.
Attachment #197417 - Flags: approval1.8b5?

Comment 33

12 years ago
Adding an i-framed forum that is really good for testing this, even though it
might be the competitor :)

Updated

12 years ago
Blocks: 296856

Comment 34

12 years ago
Comment on attachment 197417 [details] [diff] [review]
patch

Approved per 9/26 bug triage meeting.
Attachment #197417 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.