WebView.pageDown() isn't guaranteed to ever reach the bottom of the page. |
|||||||||
Issue descriptionThe CTS test android.webkit.cts.WebViewTest#testPageScroll assumes that the WebView.pageDown() API will, if called repeatedly, eventually return "false" to indicate that no downward scrolling is possible as the end of the page has been reached, which is a reasonable API assumption. However, for certain combinations of document height, view height, and display scale factor, this doesn't actually ever happen and the test times out. This can be reproduced 100% reliably by using a sailfish running P with the developer setting to simulate a "Tall" display cutout, which happens to make the CTS test window exactly the right height to trigger this bug. In that repro case, the container view height is 1521 pixels, and after calling pageDown a number of times the scrollY is 16096, but subsequent calls which attempt to scroll to 16097 (the actual lowest scroll position possible) end up rounding the scroll target down to 16096 when it converts using the device scale factor in AwContents::SmoothScroll here: https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents.cc?rcl=d694dd9d57e463490d7d52e7b4885cd4a681d64c&l=1205 This is using integer division because the function it calls takes an int parameter. BrowserViewRenderer::ScrollTo takes special steps to avoid this exact problem when calculating the offset but SmoothScroll does not: https://cs.chromium.org/chromium/src/android_webview/browser/browser_view_renderer.cc?rcl=54d36f8ec28de138daf7137771a7562314b9290c&l=571 I guess implementing something similar should fix it but I'm not familiar enough with scrolling and the math here to propose an actual change. Hopefully this is enough detail for someone else to do it :) We should aim to fix this ASAP because this is actually failing CTS for one vendor's device, but it doesn't need to be a release blocker; it doesn't fail CTS on other devices due to luck of window sizes, and any effect on real apps is probably minor at worst (since this could already be happening on many devices if the webview height is the "wrong" number and nobody has noticed).
,
Sep 6
I am kind of familiar with device scale factor because of selection handles. Looks like torne@ did the hard work for identifying the issue already, and we have a promising solution, I can try to find a slot to go through the labor part.
,
Sep 8
So the root cause was not because of the rounding error in BVR::SmoothScroll(), but in BVR::SetTotalRootLayerScrollOffset() https://cs.chromium.org/chromium/src/android_webview/browser/browser_view_renderer.cc?rcl=0e40db0cccf17d0d457e7e585bedbce50a96e026&l=638 In this computation, if (max_scroll_offset_unscaled_.y()) { scroll_offset.set_x((scroll_offset_unscaled.y() * max_offset.y()) / max_scroll_offset_unscaled_.y()); } scroll_offset_unscaled.y() = 6132.f max_offset.y() = 16097 max_scroll_offset_unscaled.y() = 6132.f and (int)(6132.f * 16097) / 6132.f) = 16096, because for 6132.f * 16097 = 98706804, we lose the precision due to float number. So ended with something like (int)16096.99934768428 -> 16096..
,
Sep 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d2b94f45531f7601d15ca6439f02b0c524b509f1 commit d2b94f45531f7601d15ca6439f02b0c524b509f1 Author: Shimi Zhang <ctzsm@chromium.org> Date: Tue Sep 11 00:55:54 2018 aw: Add rounding to BVR::SetTotalRootLayerScrollOffset() calculation. The intermediate multiplication result was too large to store in float precisely, therefore there was a rounding error. To mitigate this, we apply a rounding for the integer conversion. We only need this for scale up since the granularity is finer. Bug: 881458 Change-Id: I9c07fdfb71fcbc59f36f5094bf813053cac8d8e8 Reviewed-on: https://chromium-review.googlesource.com/1214686 Commit-Queue: Shimi Zhang <ctzsm@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#590147} [modify] https://crrev.com/d2b94f45531f7601d15ca6439f02b0c524b509f1/android_webview/browser/browser_view_renderer.cc [modify] https://crrev.com/d2b94f45531f7601d15ca6439f02b0c524b509f1/android_webview/browser/browser_view_renderer_unittest.cc
,
Sep 11
should be fixed, ran this CTS test with regular mode and tall cutoff mode, both passed.
,
Sep 11
*cutout*
,
Sep 11
This needs to be merged to M70 to get the fix out ASAP for vendors.
,
Sep 12
cc benmason@ and cmasso@ for increasing the visibility of merge request.
,
Sep 12
Approved for merge into 70, branch 3538.
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/327dabe24c34a8701610927437d5f4d27d391d9b commit 327dabe24c34a8701610927437d5f4d27d391d9b Author: Shimi Zhang <ctzsm@chromium.org> Date: Wed Sep 12 17:06:54 2018 aw: Add rounding to BVR::SetTotalRootLayerScrollOffset() calculation. The intermediate multiplication result was too large to store in float precisely, therefore there was a rounding error. To mitigate this, we apply a rounding for the integer conversion. We only need this for scale up since the granularity is finer. Bug: 881458 Change-Id: I9c07fdfb71fcbc59f36f5094bf813053cac8d8e8 Reviewed-on: https://chromium-review.googlesource.com/1214686 Commit-Queue: Shimi Zhang <ctzsm@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#590147}(cherry picked from commit d2b94f45531f7601d15ca6439f02b0c524b509f1) Reviewed-on: https://chromium-review.googlesource.com/1222188 Reviewed-by: Shimi Zhang <ctzsm@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#330} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/327dabe24c34a8701610927437d5f4d27d391d9b/android_webview/browser/browser_view_renderer.cc [modify] https://crrev.com/327dabe24c34a8701610927437d5f4d27d391d9b/android_webview/browser/browser_view_renderer_unittest.cc
,
Sep 12
,
Sep 13
Verified fixed on walleye with 70.0.3538.17 on master using default and tall cutouts. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by boliu@chromium.org
, Sep 6