Created attachment 169625 [details] Html file to reproduce On a windows 7 tablet, PAN operation(=scroll) causes corruption of screen. Does anybody know how to resolve this or have the fix? How to reproduce. 1. Prepare a HTML contents which have an element specifying other than "visible" to the property overflow in css. 2. Load the contents with webkit 3. Operate the touch operaion, PAN on the element. Problem The content in the element protrudes outside the placeholder for it and can disappear. The build version Webkit.exe on r131112 for Nightly builds. We guess Source\WebKit\win\WebView.cpp has some bugs on this issue. Here is the sample contents to reproduce problem. You will see the problem if you PAN on the field for "overflow:auto". ---------- <HTML> <HEAD><TITLE>pan with css:overflow</TITLE></HEAD> <BODY> <font size="+2"> <div style="border: 2px solid blue; padding: 5px 5px 5px 5px; overflow:visible;"> overflow:visible </div> <br> <div style="border: 2px solid red; padding: 5px 5px 5px 5px; overflow:auto;"> overflow:auto </div> </font> </BODY> </HTML> ---------- Hideki
Created attachment 169627 [details] Problem image Here is the image when the problem occurs. The lower area has a problem. There, overflow:auto is used. The upper area has no problem because overflow:visible is specified.
Created attachment 172286 [details] The patch to fix this problem (WinCairo port) Ater my team debuged this problem, we figured out that calling scrollByRecursively fuction in Source/WebKit/win/WebView.cpp in line of 1663 causes this phenomenon. The 2nd parameter for this function must be specified whether ScrollOffsetUnclamped or ScrollOffsetClamped. Because the call in the 1663rd line does not specify for the 2nd parameter, the default value ScrollOffsetUnclamped is applied. That is the reason why scroll is performed over the scroll limit area. So we change the 1663rd line to specify ScrollOffsetClamped for the 2nd parameter. We have attached the patch, please review it.
Comment on attachment 172286 [details] The patch to fix this problem (WinCairo port) Canceling r+, which can only done by reviewers. What you can do is r?.
Comment on attachment 172286 [details] The patch to fix this problem (WinCairo port) View in context: https://bugs.webkit.org/attachment.cgi?id=172286&action=review Change is probably OK, but bug fixes like this need a ChangeLog before they can be reviewed and landed. > Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:1665 > // We negate here since panning up moves the content up, but moves the scrollbar down. Not the fault of this patch, but this code is in the complete wrong place. It’s inappropriate for panning code that makes use of nodes, renderers, and such to be in the platform layer, which is a separate subsystem that is lower level. GraphicsContextCairoWin.cpp is a crazy place for this code, and this code must be deleted or moved ASAP. Longer term we may even move the platform code to a separate library, and code like this would not even be something you could compile. > Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:1666 > + m_gestureTargetNode->renderer()->enclosingLayer()->scrollByRecursively(IntSize(-deltaX, -deltaY), WebCore::RenderLayer::ScrollOffsetClamping::ScrollOffsetClamped); Should not need the WebCore:: prefix for code inside WebCore.
Created attachment 183864 [details] The modified patch to fix this problem (WinCairo port) Darin, thank you for reviewing the patch. I have added ChangeLog and removed WebCore:: prefix. About , the place to apply the patch is not Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp. It must be Source/WebKit/win/WebView.cpp. I have modified Index. Please review the modified patch.
Comment on attachment 183864 [details] The modified patch to fix this problem (WinCairo port) Attachment 183864 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16033373
Created attachment 183879 [details] The modified patch to fix buildbot warning buildbot warns about "RenderLayer::ScrollOffsetClamping::ScrollOffsetClamped" Here is the message. warning C4482: nonstandard extension used: enum 'WebCore::RenderLayer::ScrollOffsetClamping' used in qualified name C4482 warning is described as following. // C4482.cpp // compile with: /c /W1 struct S { enum E { a }; }; int i = S::E::a; // C4482 int j = S::a; // OK So I have replaced "RenderLayer::ScrollOffsetClamping::ScrollOffsetClamped" with "RenderLayer::ScrollOffsetClamped"
FWIW, I recently fixed a similar (possibly identical) bug in Blink with a one-line change in RenderLayer::scrollByRecursively(). Have a look at https://chromiumcodereview.appspot.com/14767007
Comment on attachment 183879 [details] The modified patch to fix buildbot warning I think this patch is good, but it would be better to include test cases so that we do not run into this again. Could you look at what Terry Anderson did on the Chromium port (https://chromiumcodereview.appspot.com/14767007) and perhaps use the same test case? Terry's change was at a lower level; perhaps we should do the same so that all ports would have the desired scrolling behavior, not just the Windows ports.
(In reply to Brent Fulgham from comment #9) > Comment on attachment 183879 [details] > The modified patch to fix buildbot warning > > I think this patch is good, but it would be better to include test cases so > that we do not run into this again. Could you look at what Terry Anderson > did on the Chromium port (https://chromiumcodereview.appspot.com/14767007) > and perhaps use the same test case? > > Terry's change was at a lower level; perhaps we should do the same so that > all ports would have the desired scrolling behavior, not just the Windows > ports. If we add "clamp" as third argument in this: https://github.com/WebKit/WebKit/blob/71af797599da12fa56a502b6f23501f31e289dc2/Source/WebCore/rendering/RenderLayerScrollableArea.cpp#L1891 and merge test cases from the Chromium patch, will it work? Tried to change testcase from Chromium patch into JSFiddle but it seems, it will work only in EWS: https://jsfiddle.net/ca3pouzy/show Or this is not needed anymore? Thanks!
(In reply to Ahmad Saleem from comment #10) > (In reply to Brent Fulgham from comment #9) > > Comment on attachment 183879 [details] > > The modified patch to fix buildbot warning > > > > I think this patch is good, but it would be better to include test cases so > > that we do not run into this again. Could you look at what Terry Anderson > > did on the Chromium port (https://chromiumcodereview.appspot.com/14767007) > > and perhaps use the same test case? > > > > Terry's change was at a lower level; perhaps we should do the same so that > > all ports would have the desired scrolling behavior, not just the Windows > > ports. > > If we add "clamp" as third argument in this: > > https://github.com/WebKit/WebKit/blob/ > 71af797599da12fa56a502b6f23501f31e289dc2/Source/WebCore/rendering/ > RenderLayerScrollableArea.cpp#L1891 > > and merge test cases from the Chromium patch, will it work? > > Tried to change testcase from Chromium patch into JSFiddle but it seems, it > will work only in EWS: > > https://jsfiddle.net/ca3pouzy/show > > Or this is not needed anymore? Thanks! We had "clamp" and merge this patch some time in the past but this commit reverted and removed "clamp", which seems we might not need this anymore. https://github.com/WebKit/WebKit/commit/01e728a2e5f83ce6234dae42132c7c0db7eb1f07 @Simon - this change was done by you, do you think we can close this bug now or we just land test cases from Chromium patch, if needed? Thanks!