| 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: | P2 | ||||||||||||||||||
| Version: | 523.x (Safari 3) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Brent Fulgham
2008-12-16 15:25:19 PST
Created attachment 26069 [details]
Simple test case showing the scroll not working
Created attachment 26070 [details]
Simple test case showing the scroll functioning properly
Created attachment 26071 [details]
Image of 'bad' scroll
Created attachment 26072 [details]
Image of 'bad' scroll in the reduced test case
Created attachment 26073 [details]
Image of 'good' scroll in reduced test case
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. 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/. Marshall Cullpepper and the guys on the Titanium team seemed to find a workaround: http://github.com/marshall/webkit_titanium/commit/aa1a8dce6e8df560c756701872b316736af9049d 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, ®ionBox);
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.
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. 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. 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? 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. 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. Interesting -- maybe a race condition? 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. (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. 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. Created attachment 31065 [details]
Corrects HDC/cairo_t* Context Synchronization
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);
(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). 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. Created attachment 31100 [details]
Revision based on Eric's review comments.
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.
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. (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. (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. Landed in @r44539. |