Bug 184462 – Web Automation: simulated mouse interactions should not be done until associated DOM events have been dispatched

• Component: WebDriver
• Status: RESOLVED FIXED

[Blaze Burg]

We do this properly for keyboard events but not mouse events.

[Radar WebKit Bug Importer]

<rdar://problem/39323336>

[Blaze Burg]

Created attachment 337755 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 337755 [details]
Patch

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:82
> +    std::optional<String> message { std::nullopt };

I don't think we need to initialize this, it's already nullopt. It's a bit weird to have here some members, then the constructors and then another member, could we move this below the constructors?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:84
> +    AutomationCommandError(Inspector::Protocol::Automation::ErrorMessage type)

explicit

> Source/WebKit/UIProcess/WebPageProxy.cpp:1882
> +    m_mouseEventQueue.append(event);

I don't think we want to append events if the page was closed, I would early return if !isValid()

> Source/WebKit/UIProcess/WebPageProxy.cpp:1894
> +    NativeWebMouseEvent event = m_mouseEventQueue.first();

Do we want to copy the event here? I guess it could be const NativeWebMouseEvent&

> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
> +    auto& event = m_mouseEventQueue.first();
> +    if (event.type() != WebEvent::Type::MouseDown)
> +        return nullptr;

What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5209
> +        } else {
> +            if (auto* automationSession = process().processPool().automationSession())

else if

[Blaze Burg]

Comment on attachment 337755 [details]
Patch

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

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:82
>> +    std::optional<String> message { std::nullopt };
> 
> I don't think we need to initialize this, it's already nullopt. It's a bit weird to have here some members, then the constructors and then another member, could we move this below the constructors?

I'll clean this up. I just kept appending stuff as I went.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:84
>> +    AutomationCommandError(Inspector::Protocol::Automation::ErrorMessage type)
> 
> explicit

Good catch.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1882
>> +    m_mouseEventQueue.append(event);
> 
> I don't think we want to append events if the page was closed, I would early return if !isValid()

OK.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1894
>> +    NativeWebMouseEvent event = m_mouseEventQueue.first();
> 
> Do we want to copy the event here? I guess it could be const NativeWebMouseEvent&

I'll make it take a reference.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
>> +        return nullptr;
> 
> What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?

I tested this and nothing seemed broken. I looked into this more after reading your question. The chain is as follows:

- Enqueue mousedown event
- UIProcess sends mousedown to WebProcess
- EventDispatcher tries to dispatch
-> HTMLSelectElement::menuListDefaultEventHandler
-> RenderMenuList::showPopup
-> WebPopupMenu::show
-> IPC to UIProcess
-> WebPopupMenuProxy(Mac)::showPopupMenu

At this point WebProcess is still processing the mousedown, but now execution is back in UIProcess. It will spin a nested run loop to handle the NSEvents being sent to the dropdown. When one is selected, the native control eats the subsequent mouseup that fires over the opened dialog, so no mouseup is enqueued into m_mouseEventQueue once it is dismissed. When this returns to WebProcess it makes a fake event and dispatches it within the DOM only (i.e., not a native event).

So I don't think there is anything to change here.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5209
>> +            if (auto* automationSession = process().processPool().automationSession())
> 
> else if

OK

[Blaze Burg]

Created attachment 338027 [details]
Proposed Fix

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #4)
> 
> >> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
> >> +        return nullptr;
> > 
> > What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?
> 
> I tested this and nothing seemed broken. I looked into this more after
> reading your question. The chain is as follows:
> 
> - Enqueue mousedown event
> - UIProcess sends mousedown to WebProcess
> - EventDispatcher tries to dispatch
> -> HTMLSelectElement::menuListDefaultEventHandler
> -> RenderMenuList::showPopup
> -> WebPopupMenu::show
> -> IPC to UIProcess
> -> WebPopupMenuProxy(Mac)::showPopupMenu
> 
> At this point WebProcess is still processing the mousedown, but now
> execution is back in UIProcess. It will spin a nested run loop to handle the
> NSEvents being sent to the dropdown. When one is selected, the native
> control eats the subsequent mouseup that fires over the opened dialog, so no
> mouseup is enqueued into m_mouseEventQueue once it is dismissed. When this
> returns to WebProcess it makes a fake event and dispatches it within the DOM
> only (i.e., not a native event).
> 
> So I don't think there is anything to change here.

You are right, I missed the point that events are appended but here we always get the first one, not the last one, so adding more vents doesn't affect it.

[Carlos Garcia Campos]

Comment on attachment 338027 [details]
Proposed Fix

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

This looks good to me, but needs to be approved by a WebKit2 owner.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:56
> +String AutomationCommandError::toProtocolString()

This could be const, I guess.

[Tim Horton]

Comment on attachment 338027 [details]
Proposed Fix

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1440
> +                    .setY(y - page->topContentInset())

Somehow this topContentInset() is surprising -- do we not have functions to map between whatever relevant coordinate spaces are involved?

[Blaze Burg]

Comment on attachment 338027 [details]
Proposed Fix

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

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1440
>> +                    .setY(y - page->topContentInset())
> 
> Somehow this topContentInset() is surprising -- do we not have functions to map between whatever relevant coordinate spaces are involved?

We do, in WebProcess. I don't know the equivalent from UIProcess. We do this in WebAutomationSessionProxy::computeElementLayout to go from client iframe to client main frame coordinates.

[Blaze Burg]

Committed r230743: <https://trac.webkit.org/changeset/230743>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 184747

[Blaze Burg]

(In reply to WebKit Commit Bot from comment #11)
> Re-opened since this is blocked by bug 184747

This was rolled out because clicking apparently hangs on some systems. Upon further reflection, I wrote and tested the patch with a physical USB mouse but not a trackpad. Tapping on a trackpad generates MouseForce* events and enqueues them, but we don't dequeue them in WebPageProxy::didReceiveEvent. So the fix is to move MouseForce* cases to also be handled by m_mouseEventQueue machinery.

[Blaze Burg]

Created attachment 338352 [details]
Revised Patch

[WebKit Commit Bot]

Comment on attachment 338352 [details]
Revised Patch

Clearing flags on attachment: 338352

Committed r230817: <https://trac.webkit.org/changeset/230817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 338352 [details]
Revised Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:296
> +static const char* webMouseEventTypeString(WebEvent::Type type)

This could have been:

TextStream& operator<<(TextStream& ts, WebEvent::Type type)

and then you'd use LOG_WITH_STREAM below.