Bug 187673 – Correctly adjust scroll offsets when a page is zoomed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Correctly adjust scroll offsets when a page is zoomed

[Megan Gardner]

Created attachment 345005 [details]
Patch

[Wenson Hsieh]

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

[Megan Gardner]

rdar://41712829

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 345127 [details]
Patch

[Wenson Hsieh]

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)?

[Megan Gardner]

Created attachment 345135 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 345135 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 345135 [details]
Patch for landing

Clearing flags on attachment: 345135

Committed r233883: <https://trac.webkit.org/changeset/233883>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.