We do this properly for keyboard events but not mouse events.
<rdar://problem/39323336>
Created attachment 337755 [details] Patch
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
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
Created attachment 338027 [details] Proposed Fix
(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.
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.
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?
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.
Committed r230743: <https://trac.webkit.org/changeset/230743>
Re-opened since this is blocked by bug 184747
(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.
Created attachment 338352 [details] Revised Patch
Comment on attachment 338352 [details] Revised Patch Clearing flags on attachment: 338352 Committed r230817: <https://trac.webkit.org/changeset/230817>
All reviewed patches have been landed. Closing bug.
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.