Closed Bug 129310 Opened 23 years ago Closed 19 years ago

BeOS Scrolling needs to be rewritten

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Platform:
Other
BeOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mozilla, Assigned: sergei_d)

References

(
URL
)

Details

(Keywords: fixed1.8)

Attachments

(1 file, 2 obsolete files)

patch
19.53 KB, patch
thesuckiestemail
: review+
mtschrep
: approval1.8b5+
Details | Diff | Splinter Review 
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.
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.
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.

confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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
I tried to create both, but those seem never called.
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.
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.
reminder for myself - should look at http://www.3rd-evolution.de/BeOS/MDI/
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.
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
*** Bug 275380 has been marked as a duplicate of this bug. ***
Component: General → XP Apps: GUI Features
(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? 
The blinking is still there.
(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.
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.
Attached patch Scroll and Paint rewrite (obsolete) — — Splinter Review 
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)
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;
	}
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
patch applies cleanly to CVS but breaks build.  Also verified middle-button
scrolling icon leaves a trail.

Doug, what is version you used to patch?
CVS may provide various versions.
Post full info from About page
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)
Attached patch patch (obsolete) — — Splinter Review 
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
QA Contact: doronr → nobody
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.  
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.

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.
Attached patch patch — — Splinter Review 
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)
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 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+
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.
Blocks: 266252
Attachment #197354 - Attachment is obsolete: true
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 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?
Adding an i-framed forum that is really good for testing this, even though it
might be the competitor :)
URL: http://my.opera.com/community/forums/...
Blocks: 296856
Comment on attachment 197417 [details] [diff] [review]
patch

Approved per 9/26 bug triage meeting.
Attachment #197417 - Flags: approval1.8b5? → approval1.8b5+
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: