Bug 22891

Summary:

Scrolling in Windows Cairo Broken if no background color set.

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

WebKit Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt, mculpepper, xfdbse

Priority:

Version:

523.x (Safari 3)

Hardware:

OS:

Windows XP

Attachments:

Description	Flags
Simple test case showing the scroll not working	none
Simple test case showing the scroll functioning properly	none
Image of 'bad' scroll	none
Image of 'bad' scroll in the reduced test case	none
Image of 'good' scroll in reduced test case	none
Corrects HDC/cairo_t* Context Synchronization	eric: review-
Revision based on Eric's review comments.	eric: review+

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-	Details Formatted Diff Diff
Revision based on Eric's review comments. (14.33 KB, patch) 2009-06-09 10:32 PDT, Brent Fulgham	eric: review+	Details Formatted Diff Diff
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.