Bug 155858 - Autoscrolling from a drag selection does not work in full screen, or when the window is against the screen edge
Summary: Autoscrolling from a drag selection does not work in full screen, or when the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-24 16:19 PDT by Beth Dakin
Modified: 2016-03-25 15:03 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.28 KB, patch)
2016-03-24 16:38 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (23.31 KB, patch)
2016-03-24 16:46 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (27.78 KB, patch)
2016-03-25 12:03 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (27.78 KB, patch)
2016-03-25 12:18 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (27.82 KB, patch)
2016-03-25 12:27 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2016-03-25 14:50 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2016-03-24 16:19:59 PDT
Autoscrolling from a drag selection does not work in full screen, or when the window is against the screen edge. This is an original WebKit2 regression.

rdar://problem/9338465
Comment 1 Beth Dakin 2016-03-24 16:38:52 PDT
Created attachment 274865 [details]
Patch
Comment 2 Beth Dakin 2016-03-24 16:46:50 PDT
Created attachment 274866 [details]
Patch
Comment 3 Simon Fraser (smfr) 2016-03-24 16:48:45 PDT
Comment on attachment 274865 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:1109
> +static IntSize autoscrollAdjustmentFactorForScreenBoundaries(const IntPoint& screenPoint, const FloatSize& screenSize)

This seems to be assuming that the top left of the screen is 0,0? That's not true if a secondary screen is to the left of the main monitor.
Comment 4 Beth Dakin 2016-03-25 12:03:44 PDT
Created attachment 274925 [details]
Patch
Comment 5 Beth Dakin 2016-03-25 12:18:40 PDT
Created attachment 274926 [details]
Patch
Comment 6 Beth Dakin 2016-03-25 12:27:18 PDT
Created attachment 274927 [details]
Patch
Comment 7 Simon Fraser (smfr) 2016-03-25 12:53:40 PDT
Comment on attachment 274927 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:1149
> +    CGFloat screenLeftEdge = screenRect.x();
> +    CGFloat insetScreenLeftEdge = screenLeftEdge + EDGE_DISTANCE_THRESHOLD;
> +    CGFloat screenRightEdge = screenLeftEdge + screenRect.width();
> +    CGFloat insetScreenRightEdge = screenRightEdge - EDGE_DISTANCE_THRESHOLD;
> +    if (screenPoint.x() >= screenLeftEdge && screenPoint.x() < insetScreenLeftEdge) {
> +        CGFloat distanceFromEdge = screenPoint.x() - screenLeftEdge - EDGE_DISTANCE_THRESHOLD;
> +        if (distanceFromEdge < 0)
> +            adjustmentFactor.setWidth((distanceFromEdge / EDGE_DISTANCE_THRESHOLD) * PIXELS_MULTIPLIER);
> +    } else if (screenPoint.x() >= insetScreenRightEdge && screenPoint.x() < screenRightEdge) {
> +        CGFloat distanceFromEdge = EDGE_DISTANCE_THRESHOLD - (screenRightEdge - screenPoint.x());
> +        if (distanceFromEdge > 0)
> +            adjustmentFactor.setWidth((distanceFromEdge / EDGE_DISTANCE_THRESHOLD) * PIXELS_MULTIPLIER);
> +    }
> +
> +    CGFloat screenTopEdge = screenRect.y();
> +    CGFloat insetScreenTopEdge = screenTopEdge + EDGE_DISTANCE_THRESHOLD;
> +    CGFloat screenBottomEdge = screenTopEdge + screenRect.height();
> +    CGFloat insetScreenBottomEdge = screenBottomEdge - EDGE_DISTANCE_THRESHOLD;
> +
> +    if (screenPoint.y() >= screenTopEdge && screenPoint.y() < insetScreenTopEdge) {
> +        CGFloat distanceFromEdge = screenPoint.y() - screenTopEdge - EDGE_DISTANCE_THRESHOLD;
> +        if (distanceFromEdge < 0)
> +            adjustmentFactor.setHeight((distanceFromEdge / EDGE_DISTANCE_THRESHOLD) * PIXELS_MULTIPLIER);
> +    } else if (screenPoint.y() >= insetScreenBottomEdge && screenPoint.y() < screenBottomEdge) {
> +        CGFloat distanceFromEdge = EDGE_DISTANCE_THRESHOLD - (screenBottomEdge - screenPoint.y());
> +        if (distanceFromEdge > 0)
> +            adjustmentFactor.setHeight((distanceFromEdge / EDGE_DISTANCE_THRESHOLD) * PIXELS_MULTIPLIER);
> +    }

I think you should do all this with floats. No reason to use CG types here.

You can also use screenRect.maxX() etc rather than left + width.

> Source/WebCore/platform/ios/PlatformScreenIOS.mm:127
> +    return FloatRect([[getUIScreenClass() mainScreen] _referenceBounds]);

Don't we have an implicit conversions from CGRect to FloatRect?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2342
> +    m_process->send(Messages::WebPage::WindowScreenDidChange(displayID, screenRect), m_pageID);

I'm not sure why you have to pass a screenRect here. Wen can get the screen rect from the displayID in the web process.
Comment 8 Beth Dakin 2016-03-25 14:50:04 PDT
Created attachment 274942 [details]
Patch
Comment 9 WebKit Commit Bot 2016-03-25 14:51:05 PDT
Attachment 274942 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformScreen.h:68:  The parameter name "displayID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Simon Fraser (smfr) 2016-03-25 14:57:11 PDT
Comment on attachment 274942 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:1125
> +    float insetScreenLeftEdge = screenLeftEdge + EDGE_DISTANCE_THRESHOLD;
> +    float screenRightEdge = screenRect.maxX();
> +    float insetScreenRightEdge = screenRightEdge - EDGE_DISTANCE_THRESHOLD;

You could inset the rect to get insetScreenLeftEdge, insetScreenRightEdge which is slightly cleaner.

> Source/WebCore/platform/PlatformScreen.h:68
> +    NSScreen *screenForDisplayID(PlatformDisplayID displayID);

No need for displayID name.
Comment 11 Beth Dakin 2016-03-25 15:03:39 PDT
Thanks Simon!

(In reply to comment #10)
> Comment on attachment 274942 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274942&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:1125
> > +    float insetScreenLeftEdge = screenLeftEdge + EDGE_DISTANCE_THRESHOLD;
> > +    float screenRightEdge = screenRect.maxX();
> > +    float insetScreenRightEdge = screenRightEdge - EDGE_DISTANCE_THRESHOLD;
> 
> You could inset the rect to get insetScreenLeftEdge, insetScreenRightEdge
> which is slightly cleaner.
> 

Somehow I missed this comment before I committed, but also this strikes me as a slightly more confusing way to do things. (Though maybe I am not thinking of exactly the same rect insetting code that you are?)

> > Source/WebCore/platform/PlatformScreen.h:68
> > +    NSScreen *screenForDisplayID(PlatformDisplayID displayID);
> 
> No need for displayID name.

Removed.

http://trac.webkit.org/changeset/198692