RESOLVED FIXED 22891
Scrolling in Windows Cairo Broken if no background color set. 
https://bugs.webkit.org/show_bug.cgi?id=22891
Summary Scrolling in Windows Cairo Broken if no background color set.
Brent Fulgham
Reported 2008-12-16 15:25:19 PST
Attempting to view the http://planet.webkit.org page with the WinLauncher executable on the Cairo-backend exhibits a scrolling bug (see attached image "bad_scroll_planet_webkit.jp"). As you scroll further down the page, the "bad" region at the top of the screen grows, until some inflection point at which the cycle seems to start over. I managed to reduce the problem down to the point where the scroll works as expected if the page has a background color set. Consider Bad.html (and corresponding "bad_scroll_reduced.jpg") where the scroll misbehaves, and the Good.html (and corresponding "good_scroll_reduced.jpg"), where things work properly. bfulgham@bfulgham ~ $ diff Bad.html Good.html 6c6 < <body> --- > <body bgcolor="#FFFFFF">
Attachments
Simple test case showing the scroll not working (6.58 KB, text/html)
2008-12-16 15:25 PST, Brent Fulgham 		no flags
Details
Simple test case showing the scroll functioning properly (6.59 KB, text/html)
2008-12-16 15:26 PST, Brent Fulgham 		no flags
Details
Image of 'bad' scroll (51.10 KB, image/jpeg)
2008-12-16 15:26 PST, Brent Fulgham 		no flags
Details
Image of 'bad' scroll in the reduced test case (55.47 KB, image/jpeg)
2008-12-16 15:26 PST, Brent Fulgham 		no flags
Details
Image of 'good' scroll in reduced test case (55.89 KB, image/jpeg)
2008-12-16 15:27 PST, Brent Fulgham 		no flags
Details
Corrects HDC/cairo_t* Context Synchronization (14.40 KB, patch)
2009-06-08 15:08 PDT, Brent Fulgham 		eric: review-
DetailsFormatted DiffDiff
Revision based on Eric's review comments. (14.33 KB, patch)
2009-06-09 10:32 PDT, Brent Fulgham 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Brent Fulgham
Comment 1 2008-12-16 15:25:54 PST
Created attachment 26069 [details] Simple test case showing the scroll not working
Brent Fulgham
Comment 2 2008-12-16 15:26:12 PST
Created attachment 26070 [details] Simple test case showing the scroll functioning properly
Brent Fulgham
Comment 3 2008-12-16 15:26:34 PST
Created attachment 26071 [details] Image of 'bad' scroll
Brent Fulgham
Comment 4 2008-12-16 15:26:53 PST
Created attachment 26072 [details] Image of 'bad' scroll in the reduced test case
Brent Fulgham
Comment 5 2008-12-16 15:27:11 PST
Created attachment 26073 [details] Image of 'good' scroll in reduced test case
Brent Fulgham
Comment 6 2008-12-16 15:34:50 PST
It appears that the bug persists if the CSS uses a background image, even if you set a background color. If you do not set the background image (and do not set a background color) scroll breaks. If you remove the background image and set a background color everything is fine.
Gustavo Noronha (kov)
Comment 7 2008-12-17 09:17:18 PST
Brent asked people to try if this happens with GTK+; I tested with GtkLauncher from TOT, and was unable to reproduce it with http://planet.webkit.org/.
Brent Fulgham
Comment 8 2009-02-05 14:59:45 PST
Marshall Cullpepper and the guys on the Titanium team seemed to find a workaround: http://github.com/marshall/webkit_titanium/commit/aa1a8dce6e8df560c756701872b316736af9049d
Brent Fulgham
Comment 9 2009-02-05 15:22:24 PST
Current workaround: @@ -771,7 +771,9 @@ void WebView::scrollBackingStore(FrameView* frameView, int dx, int dy, const Int // Scroll the bitmap. RECT scrollRectWin(scrollViewRect); RECT clipRectWin(clipRect); - ::ScrollDC(bitmapDC, dx, dy, &scrollRectWin, &clipRectWin, updateRegion, 0); + // fix for repaint issue - http://jira.appcelerator.org/jira/browse/TI-58 + ::ScrollWindowEx(m_viewWindow, dx, dy, &scrollRectWin, &clipRectWin, updateRegion, 0, 0); + //::ScrollDC(bitmapDC, dx, dy, &scrollRectWin, &clipRectWin, updateRegion, 0); RECT regionBox; ::GetRgnBox(updateRegion, &regionBox); I'm worried this may not always do the right thing, but perhaps there is no difference in practice. The positive Y direction in Cairo is inverted from Windows, which has caused several other bugs. Maybe something similar is wrong with the handling of the bitmap region being scrolled.
Dave Hyatt
Comment 10 2009-02-05 15:27:08 PST
That does not do the right thing. It scrolls the bits of the window itself rather than the backing store bits in the offscreen bitmap.
Dave Hyatt
Comment 11 2009-02-05 15:29:23 PST
This bug looks like there is a flipping problem. Maybe look into how the DIB is made (we flip it) and make sure Cairo understands that there is a "flip" in effect and respects it.
Marshall Culpepper
Comment 12 2009-02-05 16:25:34 PST
FYI This was a pretty crude hack we made late last night in an effort to just get it working before our release. Dave/Brent can you suggest where in the code we look for mirroring the offscreen bitmap's "flip" in Cairo?
Brent Fulgham
Comment 13 2009-02-06 11:42:49 PST
Actually, I don't see how this could be caused by flipping of the context because when a background color is set, the scrolling problem goes away. If it was purely a context orientation issue, I don't see how this simple difference would make the issue go away. I played with flipping the cairo_matrix for the DC in GraphicsContextCairo, but this just created a flipped view of the web page, which still suffered from the tearing problem. I did not try adjusting the bitmap region in the WebView code because I stopped thinking that this issue is related to the transform matrix since the presence of background color yields a correct scroll.
Brent Fulgham
Comment 14 2009-03-10 13:39:52 PDT
This bug seems to go away in Debug builds of the current WebKit tree (with Cairo 1.8.6). Unfortunately, it still happens with Release builds.
Marshall Culpepper
Comment 15 2009-03-10 14:11:00 PDT
Interesting -- maybe a race condition?
Brent Fulgham
Comment 16 2009-03-26 22:48:38 PDT
For reference: This is happening with Cairo 1.8.6 on Windows, though I also saw it with an earlier version (1.8.4 or 1.8.2) and may have existed far before that.
xfdbse
Comment 17 2009-03-26 23:20:03 PDT
(In reply to comment #16) > For reference: This is happening with Cairo 1.8.6 on Windows, though I also > saw it with an earlier version (1.8.4 or 1.8.2) and may have existed far before > that. > yeah, also happen with cairo 1.4.14 with directfb+gtk+ platform.
Brent Fulgham
Comment 18 2009-06-08 15:00:59 PDT
It turns out that this bug is specific to the Cairo Windows port, though some of the behavior seems to be mimicked by the FrameBuffer implementation. The Windows HDC was not being kept consistently in sync with the Cairo context, resulting in strange transformations and incorrect geometry computations when scrolling or otherwise interacting with screens containing mixtures of text, images, and paths. The attached patch brings the Cairo implementation more in line with the corresponding CoreGraphics implementation, and keeps the HDC in sync with the corresponding cairo_t* context.
Brent Fulgham
Comment 19 2009-06-08 15:08:11 PDT
Created attachment 31065 [details] Corrects HDC/cairo_t* Context Synchronization
Eric Seidel (no email)
Comment 20 2009-06-08 16:26:52 PDT
Comment on attachment 31065 [details] Corrects HDC/cairo_t* Context Synchronization In general this looks fine. but I'm not really a windows guy. Should follow "the create rule" and webkit naming style: 39 static cairo_t* CairoContextWithHDC(HDC hdc, bool hasAlpha) createCairoContextWithHDC(...) Does: 44 GetObject(bitmap, sizeof(info), &info); need a later release call? Extra spaces: 53 cairo_t* context = cairo_create (image); 54 cairo_surface_destroy (image); This shoudl be abstracted into its own function: 90 // Create a bitmap DC in which to draw. 91 BITMAPINFO bitmapInfo; 92 bitmapInfo.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); which returns the right BITMAPINFO. BITMAPINFO bitmapInfo = bitmapInfoForSize(destRect.size()); Should be its own function too: if (supportAlphaBlend) { 114 BITMAP bmpInfo; 115 GetObject(bitmap, sizeof(bmpInfo), &bmpInfo); 116 int bufferSize = bmpInfo.bmWidthBytes * bmpInfo.bmHeight; 117 memset(bmpInfo.bmBits, 0, bufferSize); 118 } fillWithClearColor(bitmap); Should again be another local static function: 123 // Apply a translation to our context so that the drawing done will be at (0,0) of the bitmap. 124 XFORM xform; 125 xform.eM11 = 1.0f; 126 xform.eM12 = 0.0f; 127 xform.eM21 = 0.0f; 128 xform.eM22 = 1.0f; 129 xform.eDx = -dstRect.x(); 130 xform.eDy = -dstRect.y(); XFORM xform = makeXFORM(1, 0, 0, 1, -x, -y); Breaking it all out like that will make the original method much smaller, and hopefully make it easier to understand what you're actually trying to do. Ugly (and error prone!) that we have to do this all manually: 177 // Delete all our junk. 178 cairo_surface_destroy(image); 179 ::DeleteDC(hdc); 180 ::DeleteObject(bitmap); This is confusing: return; 183 } 184 185 m_data->restore(); I think that restore shoudl be an early return, and the if block removed. More extra spaces: cairo_t* targetRef = cairo_create (image); 58 cairo_surface_destroy (image);
Brent Fulgham
Comment 21 2009-06-08 22:17:46 PDT
(In reply to comment #20) > Ugly (and error prone!) that we have to do this all manually: > 177 // Delete all our junk. > 178 cairo_surface_destroy(image); > 179 ::DeleteDC(hdc); > 180 ::DeleteObject(bitmap); I think this is a necessary evil. After searching the existing source, it seems we don't have autopointers that support device context, bitmap, or Cairo types. External libraries may provide this, but I don't think there's anything internal to WebKit that will clean this up (see the CoreGraphics version of this file for similar structure).
Brent Fulgham
Comment 22 2009-06-09 10:12:25 PDT
Thanks for the thorough review Eric. A few comments: (In reply to comment #20) > Should follow "the create rule" and webkit naming style: > 39 static cairo_t* CairoContextWithHDC(HDC hdc, bool hasAlpha) Done > Does: > 44 GetObject(bitmap, sizeof(info), &info); > > need a later release call? No -- this just files in data in the 'info' autovariable. > Extra spaces: > 53 cairo_t* context = cairo_create (image); > 54 cairo_surface_destroy (image); Done > This shoudl be abstracted into its own function: BITMAPINFO bitmapInfo = bitmapInfoForSize(destRect.size()); Done > Should be its own function too: > fillWithClearColor(bitmap); Done > Should again be another local static function: > XFORM xform = makeXFORM(1, 0, 0, 1, -x, -y); I agree with this idea, but looking over the other sources, the manual structure assignment is the standard way it's currently done. I'd like to do a second pass to refactor this and hit the five or so other files with this same issue, rather than expand the scope of this bug to address this right now. > Ugly (and error prone!) that we have to do this all manually: > 177 // Delete all our junk. > 178 cairo_surface_destroy(image); > 179 ::DeleteDC(hdc); > 180 ::DeleteObject(bitmap); Unfortunately, this is a necessary evil at the moment -- there is no autopointer available to deal with this in the current WebKit infrastructure. > This is confusing: > return; > 183 } > 184 > 185 m_data->restore(); > > I think that restore shoudl be an early return, and the if block removed. Done > More extra spaces: > cairo_t* targetRef = cairo_create (image); > 58 cairo_surface_destroy (image); Done. The revised patch includes these changes.
Brent Fulgham
Comment 23 2009-06-09 10:32:33 PDT
Created attachment 31100 [details] Revision based on Eric's review comments.
Eric Seidel (no email)
Comment 24 2009-06-09 11:14:22 PDT
Comment on attachment 31100 [details] Revision based on Eric's review comments. extra space: 80 static void fillWithClearColor (HBITMAP bitmap) and here: 93 m_data->cr = createCairoContextWithHDC (dc, hasAlpha); I really don't know what this all does. But it seems sane enough. Ideally someone with Cairo knowledge would review this, but I'm not sure we have any of those... at least any who would have time. Looks sane enough. Please fix the nits when landing.
Peter Kasting
Comment 25 2009-06-09 11:21:21 PDT
Taking myself off, I'm utterly ignorant of Cairo, low-level Windows graphics, etc. :) BTW, if you really want, you could write a little local helper autopointer class for these types. A dozen lines of C++ would do it. It might be overkill for just this patch but it'd provide an implementation that could later be moved somewhere where everyone could use it.
Brent Fulgham
Comment 26 2009-06-09 11:29:18 PDT
(In reply to comment #24) > (From update of attachment 31100 [details] [review]) > extra space: > 80 static void fillWithClearColor (HBITMAP bitmap) > > and here: > 93 m_data->cr = createCairoContextWithHDC (dc, hasAlpha); Fixed. I have *got* to stop using my work style here.
Brent Fulgham
Comment 27 2009-06-09 11:29:59 PDT
(In reply to comment #25) > BTW, if you really want, you could write a little local helper autopointer > class for these types. A dozen lines of C++ would do it. It might be overkill > for just this patch but it'd provide an implementation that could later be > moved somewhere where everyone could use it. That's a good idea. I'm preparing a refactoring patch which should include that.
Brent Fulgham
Comment 28 2009-06-09 11:32:30 PDT
Landed in @r44539.
Note You need to log in before you can comment on or make changes to this bug.