Bug 187673 - Correctly adjust scroll offsets when a page is zoomed
Summary: Correctly adjust scroll offsets when a page is zoomed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-13 17:03 PDT by Megan Gardner
Modified: 2018-07-16 21:44 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.48 KB, patch)
2018-07-13 17:07 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2018-07-16 16:07 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (6.00 KB, patch)
2018-07-16 18:43 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2018-07-13 17:03:48 PDT
Correctly adjust scroll offsets when a page is zoomed
Comment 1 Megan Gardner 2018-07-13 17:07:41 PDT
Created attachment 345005 [details]
Patch
Comment 2 Wenson Hsieh 2018-07-13 17:30:43 PDT
Comment on attachment 345005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345005&action=review

> Source/WebCore/page/ios/EventHandlerIOS.mm:595
> +    float insetScreenLeftEdge = screenLeftEdge + edgeDistanceThreashold.width;

Nit - edgeDistanceThreashold => edgeDistanceThreshold
Comment 3 Megan Gardner 2018-07-14 00:45:47 PDT
rdar://41712829
Comment 4 Wenson Hsieh 2018-07-14 20:55:05 PDT
Comment on attachment 345005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345005&action=review

> Source/WebCore/page/ios/EventHandlerIOS.mm:591
> +    CGSize edgeDistanceThreashold = CGRect(screenRect).size;

These three lines can just be `CGSize edgeDistanceThreshold = screenRect.size() / 10;`.

Not new to this patch, but I think screenPoint and screenRect are confusing names — it wasn't obvious to me that these are actually in content coordinates (when first reading this, I had assumed that these were in screen coordinates). Should these be `positionInContentCoordinates` and `unobscuredContentRect` instead?

However, as Tim pointed out in https://bugs.webkit.org/show_bug.cgi?id=182815, it seems a bit weird that this threshold would be so different depending on screen size (i.e. it would be much easier to get to the autoscroll margins on the largest iPad Pro, and more difficult on the smallest iPhone)...I think a fixed distance around the edges avoids this problem, but since screenPoint and screenRect are in content coordinates, we need to adjust for the scale here.
Comment 5 Megan Gardner 2018-07-16 16:07:16 PDT
Created attachment 345127 [details]
Patch
Comment 6 Wenson Hsieh 2018-07-16 18:12:26 PDT
Comment on attachment 345127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345127&action=review

This looks reasonable to me, with a few minor nits.

> Source/WebCore/ChangeLog:11
> +        Instead of just setting the offset arbitrarily to 100, use the screen rect

We should update the ChangeLog to reflect the new changes.

> Source/WebCore/page/ios/EventHandlerIOS.mm:593
> +    CGSize edgeDistanceThreshold = CGRect(unobscuredContentRect).size;

Can we just CGSizeMake(EDGE_DISTANCE_THRESHOLD / zoomFactor, EDGE_DISTANCE_THRESHOLD / zoomFactor)?
Comment 7 Megan Gardner 2018-07-16 18:43:23 PDT
Created attachment 345135 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2018-07-16 19:39:23 PDT
The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2018-07-16 19:39:24 PDT
The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2018-07-16 20:38:30 PDT
The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2018-07-16 20:38:31 PDT
The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2018-07-16 21:44:29 PDT
Comment on attachment 345135 [details]
Patch for landing

Clearing flags on attachment: 345135

Committed r233883: <https://trac.webkit.org/changeset/233883>
Comment 13 WebKit Commit Bot 2018-07-16 21:44:31 PDT
All reviewed patches have been landed.  Closing bug.