Issue metadata
Sign in to add a comment
|
Regression : Scrolling the PDF file by using mouse wheel is extremely slow.
Reported by
avsha...@etouch.net,
Jun 16 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version : 61.0.3132.0 (Official Build) 407583a866bb1839b09289b5f2e856b15700ea51-refs/heads/master@{#479900} 32/64 bit OS : Windows(7,8,10), Linux(14.04 LTS) Test URL : https://msu.edu/~urban/sme865/resources/embedded_pdf.html What steps will reproduce the problem? 1. Launch chrome, navigate to above test URL and scroll down the page completely. 2. Select complete URL from omnibox and drag & drop it on the vertical scroll bar of the PDF (kindly review an attached screen cast for the reference). 3. Now try to scroll the PDF by rolling the mouse wheel and observe the scrolling of PDF. Actual : Scrolling the PDF file by using mouse wheel is extremely slow after performing drag & drop action. Expected : Instead, scrolling the PDF using mouse wheel should be quick. This is a regression issue broken in ‘M-59’, below is the Manual Regression range and will soon update other info. Good build : 59.0.3062.0 Bad build : 59.0.3063.0 Note : 1. Above issue can be reproduced on any PDF file. 2. Issue is not reproducible on Mac (10.11.6, 10.12.3) OS.
,
Jun 16 2017
Confirmed this issue is from my patch.
,
Jun 16 2017
1. The first patch I change scrollbar hit test: 524f6ac4628d3d5249d291314c6100bd0a337053, I removed the innerElement of hitTestResult when mouse over a scrollbar. 2. The patch before 524f6ac4628d3d5249d291314c6100bd0a337053 is 778993dda4a5c8c4a15df8d8048165aa28b80ada, mouse over the pdf's scrollbar, innerElement of hitTestResult is "EMBED". And I can not reproduce this issue in this patch. 3. 2275c1f6d1f074d065de4d0d1aef624f9fcefa66 is the patch which first see this issue, in this patch we change mouse over scrollbar also change the innerElement of hitTestResult to scrollbar's parent. For this case, mouse over the pdf's scrollbar, innerElement of hitTestResult is "EMBED". Comparing the 2 and 3, I suspect pdf or plugin drag and drop changed in 778993dda4a5c8c4a15df8d8048165aa28b80ada...2275c1f6d1f074d065de4d0d1aef624f9fcefa66. bokan@ dtapuska@ can you add pdf or plugin folks in this issue? Thank you.
,
Jun 16 2017
wjmaclean@, dsinclair@ any ideas? chaopeng@ you likely want to do a bisect applying your latest patch on top then. Unfortunately you'll need to build at each bisect location so I suggest choosing them wisely.
,
Jun 19 2017
That's a huge commit range, without narrowing it down it would be hard to pinpoint anything. I don't know of anything off hand that changed in PDFium regarding drag and drop. It's possible there was something in webview that changed. A narrowed bisect is needed I think.
,
Jun 19 2017
Yes, this is a huge range, and I am not able to build some old version. Getting some unrelated compile error. I apply changes in 2275c1f6d1f074d065de4d0d1aef624f9fcefa66 to the patch before all my hover patch (778993dda4a5c8c4a15df8d8048165aa28b80ada). It can not reproduce this issue. So it must be something about embed/pdf changes in that range. Anyway I back to my change and found the line cause this issue. In LayoutView.cpp https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutView.cpp?rcl=eae55d25e38017cd92692689e2382c00b3329b55&l=156 ``` if (result.GetScrollbar()) { // <- (1) result.SetInnerNode(nullptr); result.SetURLElement(nullptr); ScrollableArea* scrollable_area = result.GetScrollbar()->GetScrollableArea(); if (scrollable_area && scrollable_area->GetLayoutBox() && scrollable_area->GetLayoutBox()->GetNode()) { Node* node = scrollable_area->GetLayoutBox()->GetNode(); if (node->IsDocumentNode()) node = node->GetDocument().documentElement(); result.SetInnerNode(node); result.SetURLElement(node->EnclosingLinkEventParentOrSelf()); } } ``` For mouse move over <embed> scrollbar, at (1) innerNode is null. But my change assign the scrollbar's parent to it. I am not sure should I always set the parent to innerNode.
,
Jul 6 2017
Chatted about this with Chaopeng, there's no clear reason yet, I'll take a look.
,
Jul 11 2017
A friendly reminder that M61 branch is coming soon on 07/20! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix ASAP to trunk. This way we branch M61 from a high quality trunk. Thank you.
,
Jul 13 2017
Just to Update,
Able to reproduce the issue on Windows 10 with chrome Stable #59.0.3071.115, Beta #60.0.3112.66, Dev #61.0.3153.0, Canary #61.0.3155.0
bokan@ Could you please look into it as this issue marked as RB_Stable label
,
Jul 13 2017
Yes, I will investigate shortly.
,
Jul 18 2017
,
Jul 18 2017
To update: it looks like this now activates drag and drop autoscroll in a broken way from the PDF process where it didn't before. When dropped over a scrollbar, autoscroll is activated indefinitely so it constantly cancels any other animations so that's why wheel scrolling is slow - the animation ticks one frame and then is cancelled. Still digging to get to the correct fix, hope to have something by EOD.
,
Jul 19 2017
Took a bit longer than I thought but patch is up @ https://chromium-review.googlesource.com/c/577627/. Just waiting to run through the trybots and I'll put it up for review.
,
Jul 24 2017
Just to Update,
Able to reproduce the issue on Windows 10 with chrome Canary #62.0.3164.0
Thank You...
,
Jul 25 2017
Fix is currently in the CQ. I'll request a merge when it's confirmed in Canary.
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9badf66d12584bd4967646449ca225ad02f08400 commit 9badf66d12584bd4967646449ca225ad02f08400 Author: David Bokan <bokan@chromium.org> Date: Tue Jul 25 18:15:24 2017 Cancel drag when drop doesn't navigate. Dropping a URL on a plugin doesn't cause a navigation. The drag and drop code, however, didn't bother clearing its state when a URL was dropped since a navigation would usually clear out the entire page. The bug was causing the "slow" scrolling of wheels because the autoscroll controller was constantly canceling active animations. This happens because the EventHandler would put the AutoscrollController into "drag" mode but it would never cause it to exit when the URL was dropped. https://crrev.com/2784313002 exposed this bug because dragging over a scrollbar would previously return nullptr from the hit test so we never put the AutoscrollController into "drag" mode. However, we just got lucky as the <embed> for a PDF is `position: fixed` and so it prevents autoscrolling but this is a more general problem. The fix here is to simply tell the EventHandler, in the case of a non-navigating URL drop, to clear out the relevant state. Technically we should be sending dragleave events but we'll leave that to a future patch. Bug: 733996 Change-Id: Idcf8f502de1a895e6fef4c9be0cfa0edd7204d49 Reviewed-on: https://chromium-review.googlesource.com/577627 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#489374} [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/html/HTMLSelectElement.cpp [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/input/EventHandler.h [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/page/AutoscrollController.cpp [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/page/AutoscrollController.h [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/page/DragController.cpp [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/page/DragController.h [modify] https://crrev.com/9badf66d12584bd4967646449ca225ad02f08400/third_party/WebKit/Source/core/page/DragControllerTest.cpp
,
Jul 26 2017
URGENT - PTAL. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the M61 branch #3163 ASAP to have enough baking time in Beta before Stable promotion. Thank you! Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label.
,
Jul 26 2017
The fix is in, it should be in Canary, I'll confirm tomorrow and request merge.
,
Jul 27 2017
Rechecked this issue on chrome version 62.0.3168.0 on Windows 10 and Ubuntu 14.04. Fix is working as intended. Able to scroll the PDF smoothly after the drag and drop operation performed. Adding TE-verified labels for M62
,
Jul 27 2017
Requesting merge re:#19
,
Jul 28 2017
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 30 2017
Pls merge you change to M61 branch 3163 before 3:00 PM PT on Monday so we can take it in for next week last M61 Dev release. Thank you.
,
Jul 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49dce3fb446e414fd7fd093e149a8de846482430 commit 49dce3fb446e414fd7fd093e149a8de846482430 Author: David Bokan <bokan@chromium.org> Date: Mon Jul 31 13:07:35 2017 Cancel drag when drop doesn't navigate. Dropping a URL on a plugin doesn't cause a navigation. The drag and drop code, however, didn't bother clearing its state when a URL was dropped since a navigation would usually clear out the entire page. The bug was causing the "slow" scrolling of wheels because the autoscroll controller was constantly canceling active animations. This happens because the EventHandler would put the AutoscrollController into "drag" mode but it would never cause it to exit when the URL was dropped. https://crrev.com/2784313002 exposed this bug because dragging over a scrollbar would previously return nullptr from the hit test so we never put the AutoscrollController into "drag" mode. However, we just got lucky as the <embed> for a PDF is `position: fixed` and so it prevents autoscrolling but this is a more general problem. The fix here is to simply tell the EventHandler, in the case of a non-navigating URL drop, to clear out the relevant state. Technically we should be sending dragleave events but we'll leave that to a future patch. TBR=bokan@chromium.org (cherry picked from commit 9badf66d12584bd4967646449ca225ad02f08400) Bug: 733996 Change-Id: Idcf8f502de1a895e6fef4c9be0cfa0edd7204d49 Reviewed-on: https://chromium-review.googlesource.com/577627 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#489374} Reviewed-on: https://chromium-review.googlesource.com/593849 Reviewed-by: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#148} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/html/HTMLSelectElement.cpp [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/input/EventHandler.h [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/page/AutoscrollController.cpp [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/page/AutoscrollController.h [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/page/DragController.cpp [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/page/DragController.h [modify] https://crrev.com/49dce3fb446e414fd7fd093e149a8de846482430/third_party/WebKit/Source/core/page/DragControllerTest.cpp
,
Jul 31 2017
Sorry for the delay, was on vacation Friday. Patch is now merged to 61 branch. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by msrchandra@chromium.org
, Jun 16 2017Owner: chaopeng@chromium.org
Status: Assigned (was: Unconfirmed)