Bug 99842

Summary:

Touch operation corrupts screen when specifying other than overflow:visible in css

Product:

WebKit

Reporter:

Hideki <yoshida-hxa>

Component:

WebKit Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

ahmad.saleem792, bfulgham, bweinstein, cdumez, hirano-txb, roger_fong, simon.fraser, syoichi, tdanderson, yoshida-hxa

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows 7

Attachments:

Description	Flags
Html file to reproduce	none
Problem image	none
The patch to fix this problem (WinCairo port)	darin: review-, darin: commit-queue-
The modified patch to fix this problem (WinCairo port)	buildbot: commit-queue-
The modified patch to fix buildbot warning	bfulgham: review-, bfulgham: commit-queue-

Hideki

Reported 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Hideki

Comment 1 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.

Hideki

Comment 2 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.

Hajime Morrita

Comment 3 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?.

Darin Adler

Comment 4 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.

Hideki

Comment 5 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.

Build Bot

Comment 6 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

Hideki

Comment 7 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"

Terry Anderson

Comment 8 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

Brent Fulgham

Comment 9 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.

Ahmad Saleem

Comment 10 2022-09-25 13:04:54 PDT

(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!

Ahmad Saleem

Comment 11 2022-10-16 10:32:16 PDT

(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!

Note You need to log in before you can comment on or make changes to this bug.