TouchSelection Handles are wrong after scroll for PDFs |
|||||||||
Issue descriptionRepro: i) Open Chrome on a system with a touch screen. ii) Load any PDF with text. iii) Long press over some text to select it. The touch-selection handles should appear. iv) Touch scroll the PDF, the text selection handles should re-appear. Expected: the handles should move together with the selected text. Actual: the handles remain in the same screen position as before the scroll, and are no longer aligned with the selected text. This was regressed by the CL in https://chromium-review.googlesource.com/c/chromium/src/+/782404, which I don't quite understand as it was meant to be a solution to this very problem, and I can't imagine we didn't test it before landing. But not sure what else might have changed.
,
Apr 13 2018
Able to reproduce this issue on Ubuntu 17.10 and Windows 10 on touchscreen laptop on the build without fix 66.0.3359.106 and the issue is fixed on the latest Canary 67.0.3396.0. On opening a pdf and selecting a text by long pressing on the screen and scrolling through the page, can observe that the touch-selection handles are placed at the correct position where the text was selected. Attached is the screen cast for reference. Hence adding TE verified labels as the fix is working as intended. Thanks..
,
Apr 13 2018
,
Apr 13 2018
Not sure if it's too late to get this bug fix in M66 or not ... let me know.
,
Apr 13 2018
This bug requires manual review: We are only 3 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 13 2018
How safe is this merge?
,
Apr 13 2018
Assuming no merge conflicts (which is pretty unlikely, but detectable), I think it's very safe. Of course, I'm happy to let this bake over the weekend to be sure.
,
Apr 16 2018
How does the change look today in canary? + Josafat@ for CROS review (since this is affecting touchscreen devices)
,
Apr 16 2018
checking more with wjmaclean@, this is present in M65, no external reports yet, and the trigger case is only when an item in PDF is highlighted and then scrolled. Since there won't be too much impact on Desktop, not including it for our M66 stable release tomorrow. Josafat@ - this has more impact on CROS, so please review.
,
Apr 17 2018
Let's validate on lower channels (dev) first, we can re assess the merge once bake/verification is done
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/04445ad561d99ba5fd6718e0a5f78a8452da7eda commit 04445ad561d99ba5fd6718e0a5f78a8452da7eda Author: W. James MacLean <wjmaclean@chromium.org> Date: Thu Apr 12 22:46:24 2018 Fix TouchSelection handles for PDF The fix for https://crbug.com/784932 seems to have broken the touch selection handles for PDF; sfter scrolling they're in the wrong place. Going back to the original solution, but this time making sure that on scroll we don't update the selected text, thus avoiding the DCHECK that was at the root of 784932. Bug: 829406 Change-Id: I387d120d105c351d80c337ba1646a3cb1eaf40dc Reviewed-on: https://chromium-review.googlesource.com/998073 Reviewed-by: dsinclair <dsinclair@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/heads/master@{#550398} [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/components/pdf/common/pdf.mojom [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/components/pdf/renderer/pepper_pdf_host.cc [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/components/pdf/renderer/pepper_pdf_host.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/pdf/out_of_process_instance.cc [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/pdf/pdfium/pdfium_engine.cc [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/pdf/pdfium/pdfium_engine.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/c/private/ppb_pdf.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/cpp/private/pdf.cc [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/cpp/private/pdf.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/proxy/pdf_resource.cc [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/proxy/pdf_resource.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/proxy/ppapi_messages.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/thunk/ppb_pdf_api.h [modify] https://crrev.com/04445ad561d99ba5fd6718e0a5f78a8452da7eda/ppapi/thunk/ppb_pdf_thunk.cc
,
Apr 25 2018
Rejecting merge since this is present in M65 as well. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Apr 12 2018