New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733996 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression



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 description

Chrome 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.
 
Actual_Scrolling.mp4
1.6 MB View Download
Expected_Scrolling.mp4
2.0 MB View Download
Labels: hasbisect-per-revision ReleaseBlock-Stable
Owner: chaopeng@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good build: 59.0.3062.0 (Revision: 461587).
Bad build : 59.0.3063.0 (Revision: 461928).

You are probably looking for a change made after 461849 (known good), but no later than 461850 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/05267be8219e262196306d78d7eade8083de7595..2275c1f6d1f074d065de4d0d1aef624f9fcefa66

@chaopeng: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
Adding RB Label as this is a recent Regression.
Thank You.
Cc: bokan@chromium.org
Confirmed this issue is from my patch.
Cc: dtapu...@chromium.org
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.
Cc: wjmaclean@chromium.org dsinclair@chromium.org
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.
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.
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.

Comment 7 by bokan@chromium.org, Jul 6 2017

Cc: chaopeng@chromium.org
Owner: bokan@chromium.org
Chatted about this with Chaopeng, there's no clear reason yet, I'll take a look.

Comment 8 by gov...@chromium.org, 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.
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

Comment 10 by bokan@chromium.org, Jul 13 2017

Yes, I will investigate shortly.

Comment 11 by bokan@chromium.org, Jul 18 2017

Status: Started (was: Assigned)

Comment 12 by bokan@chromium.org, 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.

Comment 13 by bokan@chromium.org, 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.
Just to Update, 
              Able to reproduce the issue on Windows 10 with chrome Canary #62.0.3164.0

Thank You...

Comment 15 by bokan@chromium.org, Jul 25 2017

Fix is currently in the CQ. I'll request a merge when it's confirmed in Canary.
Project Member

Comment 16 by bugdroid1@chromium.org, 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

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.

Comment 18 by bokan@chromium.org, Jul 26 2017

The fix is in, it should be in Canary, I'll confirm tomorrow and request merge.
Labels: TE-Verified-M62 TE-Verified-62.0.3168.0
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

Comment 20 by bokan@chromium.org, Jul 27 2017

Labels: Merge-Request-61
Requesting merge re:#19
Project Member

Comment 21 by sheriffbot@chromium.org, Jul 28 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
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

Comment 22 Deleted

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.
Project Member

Comment 24 by bugdroid1@chromium.org, Jul 31 2017

Labels: -merge-approved-61 merge-merged-3163
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

Comment 25 by bokan@chromium.org, Jul 31 2017

Status: Fixed (was: Started)
Sorry for the delay, was on vacation Friday.

Patch is now merged to 61 branch.

Sign in to add a comment