Bug 184344 – WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is directly accessing NSScreen

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

The WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll method is retrieving NSScreen information directly. These calls should be brokered.

[Radar WebKit Bug Importer]

<rdar://problem/39224969>

[Brent Fulgham]

Created attachment 337403 [details]
Patch

[EWS Watchlist]

Comment on attachment 337403 [details]
Patch

Attachment 337403 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7231943

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/events/no-scroll-on-input-text-selection.html
fast/forms/select-multiple-elements-with-mouse-drag.html
fast/events/autoscroll-in-iframe.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/forms/select/listbox-disabled-no-autoscroll.html
fast/events/autoscroll-in-iframe-body.html

[EWS Watchlist]

Created attachment 337408 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 337403 [details]
Patch

Attachment 337403 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7232000

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/events/no-scroll-on-input-text-selection.html
fast/forms/select-multiple-elements-with-mouse-drag.html
fast/events/autoscroll-in-iframe.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/forms/select/listbox-disabled-no-autoscroll.html
fast/events/autoscroll-in-iframe-body.html

[EWS Watchlist]

Created attachment 337409 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Brent Fulgham]

Created attachment 337430 [details]
Patch

[Brent Fulgham]

Created attachment 337515 [details]
Patch

[Brent Fulgham]

Created attachment 337521 [details]
Patch

[Per Arne Vollan]

Comment on attachment 337515 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:1169
> +    auto frame = toUserSpaceForPrimaryScreen(screenProperties(page->chrome().displayID()).screenRect);

Will the screen properties map be empty on WK1?

[Brent Fulgham]

(In reply to Per Arne Vollan from comment #10)
> Comment on attachment 337515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337515&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:1169
> > +    auto frame = toUserSpaceForPrimaryScreen(screenProperties(page->chrome().displayID()).screenRect);
> 
> Will the screen properties map be empty on WK1?

Oh, you are right. This will crash in WebKitLegacy. :-(

[Brent Fulgham]

Created attachment 337529 [details]
Patch

[Brent Fulgham]

Created attachment 337535 [details]
Patch

[Per Arne Vollan]

Comment on attachment 337535 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:-1167
> -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);

It seems the original transformation will be incorrect if the window is not on the first screen, since the NSWindow parameter is nil here. Should we also address that in this patch?

[Brent Fulgham]

(In reply to Per Arne Vollan from comment #14)
> Comment on attachment 337535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337535&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:-1167
> > -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);
> 
> It seems the original transformation will be incorrect if the window is not
> on the first screen, since the NSWindow parameter is nil here. Should we
> also address that in this patch?

Which NSWindow would we be passing as the second argument? If you know the right answer one to use, I’m in favor of fixing it now.

[Brent Fulgham]

Comment on attachment 337535 [details]
Patch

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

>>> Source/WebCore/page/mac/EventHandlerMac.mm:-1167
>>> -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);
>> 
>> It seems the original transformation will be incorrect if the window is not on the first screen, since the NSWindow parameter is nil here. Should we also address that in this patch?
> 
> Which NSWindow would we be passing as the second argument? If you know the right answer one to use, I’m in favor of fixing it now.

Actually, I think it is correct as-is. A nil window was used by the other eiginslncode to compute coordinates with respect to the primary display (I.e., not “no display”). I think that is the correct transformation.

[Per Arne Vollan]

Comment on attachment 337535 [details]
Patch

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

Very nice! r=me.

> Source/WebKit/WebProcess/WebProcess.cpp:138
> +#if PLATFORM(MAC)
> +#include <WebCore/PlatformScreen.h>
> +#endif

This include might not be needed.

[Brent Fulgham]

Comment on attachment 337535 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:138
>> +#endif
> 
> This include might not be needed.

You are right. I'll remove it.

[Brent Fulgham]

Committed r230454: <https://trac.webkit.org/changeset/230454>