Bug 155858 – Autoscrolling from a drag selection does not work in full screen, or when the window is against the screen edge

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

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

[Beth Dakin]

Created attachment 274865 [details]
Patch

[Beth Dakin]

Created attachment 274866 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

Created attachment 274925 [details]
Patch

[Beth Dakin]

Created attachment 274926 [details]
Patch

[Beth Dakin]

Created attachment 274927 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

Created attachment 274942 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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.

[Beth Dakin]

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