Closed Bug 419270 Opened 14 years ago Closed 14 years ago

Elements with position:fixed jump while scrolling

Categories

(Core :: Web Painting, defect)

x86
OS/2
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: wuno)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

977 bytes, patch
mozilla
: review+
beltzner
: approval1.9b4+
Details | Diff | Splinter Review
Page elements that are set to be fixed using CSS position:fixed should really stay fixed while scrolling. On Linux trunk builds and OS/2 branch builds they do, on OS/2 trunk builds they don't. 

Pages that show the problem:
- http://ninjakitten.us
  The whole header jumps around when scrolling
- http://weilbacher.org/Mozilla/builds.html
  While the fixed content (the yellow menu on the left in the default style)
  does stay fixed, the surrounding black border jumps around when scrolling.

I suspect that both of these are related and that this has something to do with cairo GFX or a widget change that we missed on OS/2 but I don't really know where to start debugging this...
(In reply to comment #0)
> I suspect that both of these are related and that this has something to do with
> cairo GFX or a widget change that we missed on OS/2 but I don't really know
> where to start debugging this...
> 
Its not cairo, because minefield-alpha2 has the same problem and IIRC this was still "normal" gfx. BTW, minefield-alpha2 is terribly slow on ninjakitten.us
Attached file this works (obsolete) —
searching through widget\src I found bug343430 that sounds very similar. A fix was done for linux and windows. Actually, the patch here tries to do the same for OS/2 what was done for windows (see https://bugzilla.mozilla.org/attachment.cgi?id=265542). So far it works.
Attachment #305403 - Flags: review?(mozilla)
Here it works to fix the bug in question but causes heavy repaint problems in the other parts of the pages that contain position:fixed elements. Not sure if that is because I have other stuff in my tree, because the patch itself looks fine to me.
Comment on attachment 305403 [details]
this works

clearing request, sigh, you're right this works only with one element. Here is a better testcase with two fixed elements
http://www.howtocreate.co.uk/fixedPosition.html
I tried also the gtk2 solution in the attachment from bug343430 https://bugzilla.mozilla.org/attachment.cgi?id=265542
This would work with the testcases mentioned, also with two fixed elements.... but then URLs started from the menus don't get called with a single left button click only with the right button using the context menu. Other menu functions like bookmark this page do work with the left mouse button.
Attachment #305403 - Attachment is patch: false
Attachment #305403 - Flags: review?(mozilla)
Attached patch alternativeSplinter Review
Tried the gtk2 solution again when I realized that the menu problem was not caused by the patch but was probably related to - Bug 419549 - click bookmark item does not work. I'm not going for review now, but maybe Peter you have some time to test it, maybe with even a better testcase. Scrolling of other elements seems to be affected by this solution (to be seen best with the test page posted in my last comment), the monospace textboxes scroll delayed.
Attachment #305403 - Attachment is obsolete: true
Comment on attachment 305832 [details] [diff] [review]
alternative

Yes, from quick testing with a Firefox debug build this seems to work. Will put it in the SeaMonkey that I use daily to see if it causes anything bad.
Comment on attachment 305832 [details] [diff] [review]
alternative

OK, went to lots of position:fixed files and it seems to work for all of them. Although I really don't understand why this time the GTK2 solution would work and not the Windows one that we use in most other cases...
Attachment #305832 - Flags: review+
Walter, when you do the work and upload a patch you should in most cases tick the "take bug" box, so that everyone can see how much time you devote to all this. :-)
Assignee: nobody → wuno
Component: General → Layout: View Rendering
Comment on attachment 305832 [details] [diff] [review]
alternative

OS/2 only change in a cross-platform file.
Attachment #305832 - Flags: approval1.9b4?
Comment on attachment 305832 [details] [diff] [review]
alternative

OS2-only fix, a1.9b4=beltzner
(In reply to comment #7)
> (From update of attachment 305832 [details] [diff] [review])
> Although I really don't understand why this time the GTK2 solution would work
> and not the Windows one that we use in most other cases...
> 
Maybe I've should have posted the comment of the checkin for this bug:

Bug 343430. Reduce the area we scroll on Windows to reduce flicker by excluding areas that shouldn't be moving. On Linux, since we can't control the area we scroll, just disable accelerated scrolling in that case and repaint everything

I guess the gtk2 version works for OS/2 since it's a generic solution. Could it be that the windows solution didn't work or worked only partially, because there is an interference with the drag'n'drop in the os2 function? Here's the diff of that respective part of nsWindow.cpp

--- widget/src/windows/nsWindow.cpp      2008-02-22 08:13:31.000000000 +0100
+++ widget/src/os2/nsWindow.cpp  2008-02-09 23:26:06.000000000 +0100
@@ -2885,49 +2261,77 @@
 //XXX Scroll is obsolete and should go away soon
 NS_METHOD nsWindow::Scroll(PRInt32 aDx, PRInt32 aDy, nsRect *aClipRect)
 {
-  RECT  trect;
+  RECTL rcl;
 
   if (nsnull != aClipRect)
   {
-    trect.left = aClipRect->x;
-    trect.top = aClipRect->y;
-    trect.right = aClipRect->XMost();
-    trect.bottom = aClipRect->YMost();
-  }
+    rcl.xLeft = aClipRect->x;
+    rcl.yBottom = aClipRect->y + aClipRect->height;
+    rcl.xRight = rcl.xLeft + aClipRect->width;
+    rcl.yTop = rcl.yBottom + aClipRect->height;
+    NS2PM( rcl);
+    // this rect is inex
+  }
+
+    // this prevents screen corruption while scrolling during a
+    // Moz-originated drag - during a native drag, the screen
+    // isn't updated until the drag ends. so there's no corruption
+  HPS hps = 0;
+  CheckDragStatus(ACTION_SCROLL, &hps);
+
+  WinScrollWindow( mWnd, aDx, -aDy, aClipRect ? &rcl : 0, 0, 0,
+                   0, SW_SCROLLCHILDREN | SW_INVALIDATERGN);
+  Update();
+
+  if (hps)
+    ReleaseIfDragHPS(hps);
 
-  ::ScrollWindowEx(mWnd, aDx, aDy, NULL, (nsnull != aClipRect) ? &trect : NULL,
-                   NULL, NULL, SW_INVALIDATE | SW_SCROLLCHILDREN);
-  // Invalidate all child windows that aren't ours; we're moving them, and we
-  // expect them to be painted at the new location even if they're outside the
-  // region we're bit-blit scrolling. See bug 387701.
-  ::EnumChildWindows(GetWindowHandle(), nsWindow::InvalidateForeignChildWindows, NULL);
-  ::UpdateWindow(mWnd);
   return NS_OK;
 }
 
 NS_IMETHODIMP nsWindow::ScrollWidgets(PRInt32 aDx, PRInt32 aDy)

Attachment #305832 - Flags: approval1.9b4? → approval1.9b4+
Walter, no, I don't think so. I looked at CheckDragStatus and that really only does something if you drag while you scroll. Unless gDragStatus would get set by mistake at some point (but then lots of other stuff would be messed up).

So, I've checked in fix from attachment 305832 [details] [diff] [review] now. In case you keep experimenting and find a better solution, please reopen.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.