Bug 99842 - Touch operation corrupts screen when specifying other than overflow:visible in css
Summary: Touch operation corrupts screen when specifying other than overflow:visible i...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-19 08:37 PDT by Hideki
Modified: 2013-07-12 09:28 PDT (History)
8 users (show)

See Also:


Attachments
Html file to reproduce (329 bytes, text/html)
2012-10-19 08:37 PDT, Hideki
no flags Details
Problem image (11.33 KB, image/png)
2012-10-19 08:42 PDT, Hideki
no flags Details
The patch to fix this problem (WinCairo port) (854 bytes, patch)
2012-11-05 00:45 PST, Hideki
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
The modified patch to fix this problem (WinCairo port) (1.67 KB, patch)
2013-01-21 18:20 PST, Hideki
buildbot: commit-queue-
Details | Formatted Diff | Diff
The modified patch to fix buildbot warning (1.65 KB, patch)
2013-01-21 20:36 PST, Hideki
bfulgham: review-
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hideki 2012-10-19 08:37:15 PDT
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
Comment 1 Hideki 2012-10-19 08:42:05 PDT
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.
Comment 2 Hideki 2012-11-05 00:45:40 PST
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 3 Hajime Morrita 2012-11-05 02:37:34 PST
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 4 Darin Adler 2013-01-16 13:45:24 PST
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.
Comment 5 Hideki 2013-01-21 18:20:22 PST
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 6 Build Bot 2013-01-21 18:34:35 PST
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
Comment 7 Hideki 2013-01-21 20:36:05 PST
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"
Comment 8 Terry Anderson 2013-07-12 08:35:57 PDT
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 9 Brent Fulgham 2013-07-12 09:28:15 PDT
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.