Bug 143749 – Force mouse events should go through normal mouse event handling code paths

• Component: WebKit2
• Status: RESOLVED FIXED

[Beth Dakin]

Force mouse events should go through normal mouse event handling code paths, and they should definitely not use the cached actionMenuHitTestResult for all of the events.

rdar://problem/20472895

[Beth Dakin]

Created attachment 250777 [details]
Patch

[WebKit Commit Bot]

Attachment 250777 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/WebEventFactory.mm:163:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/mac/WebEventFactory.mm:188:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/replay/UserInputBridge.h:81:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/replay/UserInputBridge.h:82:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/replay/UserInputBridge.h:83:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 250777 [details]
Patch

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

I would love to see some testing added in this page. I worry that if we don't add testing now, we'll never add it.

> Source/WebCore/dom/Element.cpp:257
> +    if (platformEvent.type() == PlatformEvent::MouseForceChanged && !document().hasListenerType(Document::FORCECHANGED_LISTENER))
> +        return false;
> +    if (platformEvent.type() == PlatformEvent::MouseForceDown && !document().hasListenerType(Document::FORCEDOWN_LISTENER))
> +        return false;
> +    if (platformEvent.type() == PlatformEvent::MouseForceUp && !document().hasListenerType(Document::FORCEUP_LISTENER))
> +        return false;

A little helper that maps Document::*_LISTENER to PlatformEvent::Mouse* types would make this cleaner.

> Source/WebCore/page/EventHandler.cpp:2116
> +    if (m_frameSetBeingResized)
> +        return !dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);

Weird. What's this about?

> Source/WebCore/page/EventHandler.cpp:2135
> +    if (m_frameSetBeingResized) {
> +        dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> +        return !dispatchMouseEvent(eventNames().webkitmouseforcedownEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> +    }

Same.

> Source/WebCore/page/EventHandler.cpp:2164
> +bool EventHandler::handleMouseForceUpEvent(const PlatformMouseEvent& event)
> +{
> +    RefPtr<FrameView> protector(m_frame.view());
> +
> +    setLastKnownMousePosition(event);
> +
> +    if (m_frameSetBeingResized) {
> +        dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> +        return !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> +    }
> +
> +    HitTestRequest::HitTestRequestType hitType = HitTestRequest::DisallowShadowContent | HitTestRequest::Active;
> +
> +    HitTestRequest request(hitType);
> +    MouseEventWithHitTestResults mouseEvent = prepareMouseEvent(request, event);
> +
> +    dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, mouseEvent.targetNode(), false, 0, event, false);
> +    return !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, mouseEvent.targetNode(), false, 0, event, false);
> +}

Lots of very similar-looking code here. Can it be shared?

> Source/WebCore/page/EventHandler.cpp:2178
> +bool EventHandler::handleMouseForceChangedEvent(const PlatformMouseEvent& )
> +{
> +}
> +
> +bool EventHandler::handleMouseForceDownEvent(const PlatformMouseEvent&)
> +{
> +}
> +
> +bool EventHandler::handleMouseForceUpEvent(const PlatformMouseEvent&)
> +{
> +}

These need to return false.

[Simon Fraser (smfr)]

> I would love to see some testing added in this page.
^patch

[Darin Adler]

Comment on attachment 250777 [details]
Patch

EFL build error:

Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:469:12: error: enumeration value 'MouseForceDown' not handled in switch [-Werror=switch]
Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:469:12: error: enumeration value 'MouseForceUp' not handled in switch [-Werror=switch]

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 250777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250777&action=review
> 
> I would love to see some testing added in this page. I worry that if we
> don't add testing now, we'll never add it.
> 

We will definitely get to this! I'll start to investigate.

> > Source/WebCore/dom/Element.cpp:257
> > +    if (platformEvent.type() == PlatformEvent::MouseForceChanged && !document().hasListenerType(Document::FORCECHANGED_LISTENER))
> > +        return false;
> > +    if (platformEvent.type() == PlatformEvent::MouseForceDown && !document().hasListenerType(Document::FORCEDOWN_LISTENER))
> > +        return false;
> > +    if (platformEvent.type() == PlatformEvent::MouseForceUp && !document().hasListenerType(Document::FORCEUP_LISTENER))
> > +        return false;
> 
> A little helper that maps Document::*_LISTENER to PlatformEvent::Mouse*
> types would make this cleaner.
> 

I'm not sure I know what you mean. Are you just suggesting moving this to a helper function? A helper function that specifically maps LISTENER to PlatformEvent does not sound helpful to me, but lifting these three ifs into their own helper would be fine. I feel like I'm missing something here.

> > Source/WebCore/page/EventHandler.cpp:2116
> > +    if (m_frameSetBeingResized)
> > +        return !dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> 
> Weird. What's this about?
>
> > Source/WebCore/page/EventHandler.cpp:2135
> > +    if (m_frameSetBeingResized) {
> > +        dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> > +        return !dispatchMouseEvent(eventNames().webkitmouseforcedownEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> > +    }
> 
> Same.
> 

There are lines one code like this when handling mousemoved and mouseup, which is why I added the code here. Turns out that whatever this concept is of framesets being resized is super ancient, and pre-dates EventHandler even being its own file. I could not find a sufficient explanation for what it does. Given that, and given that it is not consulted for other mouse events, I decided to remove these lines of code from the new force event handling functions.

> > Source/WebCore/page/EventHandler.cpp:2164
> > +bool EventHandler::handleMouseForceUpEvent(const PlatformMouseEvent& event)
> > +{
> > +    RefPtr<FrameView> protector(m_frame.view());
> > +
> > +    setLastKnownMousePosition(event);
> > +
> > +    if (m_frameSetBeingResized) {
> > +        dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> > +        return !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, m_frameSetBeingResized.get(), false, 0, event, false);
> > +    }
> > +
> > +    HitTestRequest::HitTestRequestType hitType = HitTestRequest::DisallowShadowContent | HitTestRequest::Active;
> > +
> > +    HitTestRequest request(hitType);
> > +    MouseEventWithHitTestResults mouseEvent = prepareMouseEvent(request, event);
> > +
> > +    dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, mouseEvent.targetNode(), false, 0, event, false);
> > +    return !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, mouseEvent.targetNode(), false, 0, event, false);
> > +}
> 
> Lots of very similar-looking code here. Can it be shared?
> 

Yes! Consolidated.

> > Source/WebCore/page/EventHandler.cpp:2178
> > +bool EventHandler::handleMouseForceChangedEvent(const PlatformMouseEvent& )
> > +{
> > +}
> > +
> > +bool EventHandler::handleMouseForceDownEvent(const PlatformMouseEvent&)
> > +{
> > +}
> > +
> > +bool EventHandler::handleMouseForceUpEvent(const PlatformMouseEvent&)
> > +{
> > +}
> 
> These need to return false.

Good point!

New patch coming soon.

[Beth Dakin]

Created attachment 250832 [details]
Patch

[Beth Dakin]

Created attachment 250849 [details]
Patch

[WebKit Commit Bot]

Attachment 250849 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:3971:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 250904 [details]
Patch with tests

The new tests need to be skipped on most platforms. I'll deal with that tomorrow.

[WebKit Commit Bot]

Attachment 250904 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:65:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:282:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:289:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:315:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:322:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/mac/EventSenderProxy.mm:348:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/Document.cpp:3971:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 7 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 250904 [details]
Patch with tests

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

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:294
> +    EventSenderPressureEvent *firstEvent = [[EventSenderPressureEvent alloc] initAtLocation:NSMakePoint(m_position.x, m_position.y)
> +                                        globalLocation:([m_testController->mainWebView()->platformWindow() convertRectToScreen:NSMakeRect(m_position.x, m_position.y, 1, 1)].origin)
> +                                        stage:1
> +                                        pressure:0.9
> +                                        phase:NSEventPhaseChanged
> +                                        time:absoluteTimeForEventTime(currentEventTime())
> +                                        eventNumber:++eventNumber];
> +    EventSenderPressureEvent *secondEvent = [[EventSenderPressureEvent alloc] initAtLocation:NSMakePoint(m_position.x, m_position.y)
> +                                        globalLocation:([m_testController->mainWebView()->platformWindow() convertRectToScreen:NSMakeRect(m_position.x, m_position.y, 1, 1)].origin)
> +                                        stage:2
> +                                        pressure:0.1
> +                                        phase:NSEventPhaseChanged
> +                                        time:absoluteTimeForEventTime(currentEventTime())
> +                                        eventNumber:++eventNumber];

Is this file using ARC? If not, these are leaked.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:327
> +    EventSenderPressureEvent *firstEvent = [[EventSenderPressureEvent alloc] initAtLocation:NSMakePoint(m_position.x, m_position.y)
> +                                        globalLocation:([m_testController->mainWebView()->platformWindow() convertRectToScreen:NSMakeRect(m_position.x, m_position.y, 1, 1)].origin)
> +                                        stage:2
> +                                        pressure:0.1
> +                                        phase:NSEventPhaseChanged
> +                                        time:absoluteTimeForEventTime(currentEventTime())
> +                                        eventNumber:++eventNumber];
> +    EventSenderPressureEvent *secondEvent = [[EventSenderPressureEvent alloc] initAtLocation:NSMakePoint(m_position.x, m_position.y)
> +                                        globalLocation:([m_testController->mainWebView()->platformWindow() convertRectToScreen:NSMakeRect(m_position.x, m_position.y, 1, 1)].origin)
> +                                        stage:1
> +                                        pressure:0.9
> +                                        phase:NSEventPhaseChanged
> +                                        time:absoluteTimeForEventTime(currentEventTime())
> +                                        eventNumber:++eventNumber];

Ditto.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:353
> +    EventSenderPressureEvent *event = [[EventSenderPressureEvent alloc] initAtLocation:NSMakePoint(m_position.x, m_position.y)
> +                                        globalLocation:([m_testController->mainWebView()->platformWindow() convertRectToScreen:NSMakeRect(m_position.x, m_position.y, 1, 1)].origin)
> +                                        stage:force < 1 ? 1 : 2
> +                                        pressure:force
> +                                        phase:NSEventPhaseChanged
> +                                        time:absoluteTimeForEventTime(currentEventTime())
> +                                        eventNumber:++eventNumber];

Ditto.

> LayoutTests/ChangeLog:10
> +        Just a few new tests. More to come.

Would be nice to get testing of at least:
* bubbling
* preventDefault

[Dean Jackson]

Comment on attachment 250904 [details]
Patch with tests

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

> Source/WebCore/page/EventHandler.cpp:2124
> +    bool swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, mouseEvent.targetNode(), false, 0, event, false);
> +    if (event.type() == PlatformEvent::MouseForceDown)
> +        swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforcedownEvent, mouseEvent.targetNode(), false, 0, event, false);
> +    if (event.type() == PlatformEvent::MouseForceUp)
> +        swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, mouseEvent.targetNode(), false, 0, event, false);

Does this mean we can only swallow the event from forcechanged if down/up also swallow?

Technically I guess you could return from within both conditionals.

> Source/WebCore/replay/UserInputBridge.cpp:163
> +bool UserInputBridge::handleMouseForceChangedEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> +{
> +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> +}
> +
> +bool UserInputBridge::handleMouseForceDownEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> +{
> +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> +}
> +
> +bool UserInputBridge::handleMouseForceUpEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> +{
> +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> +}

I haven't looked at how these are called, but is there any reason why they can't all be done in one handleMouseForceEvent? Do we need one for each type of input event name?

> Source/WebKit2/Shared/mac/WebEventFactory.mm:384
> +        if (lastPressureEvent.stage == 1 && event.stage == 2)
> +            type = WebEvent::MouseForceDown;
> +        else if (lastPressureEvent.stage == 2 && event.stage == 1)
> +            type = WebEvent::MouseForceUp;
> +        else

This is probably silly, but I wonder if we should have enums for the stage values so they read a bit better. Or are they actually just a count?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1912
> +        case PlatformEvent::MouseForceChanged:
> +            return page->corePage()->userInputBridge().handleMouseForceChangedEvent(platformMouseEvent);
> +        case PlatformEvent::MouseForceDown:
> +            return page->corePage()->userInputBridge().handleMouseForceDownEvent(platformMouseEvent);
> +        case PlatformEvent::MouseForceUp:
> +            return page->corePage()->userInputBridge().handleMouseForceUpEvent(platformMouseEvent);

Ooooh. Here's my answer to the other bit. So, considering these all do the same thing on the other end, do you think it makes sense to have a single handle function?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:73
> +    self = [super init];
> +
> +    if (self) {
> +        _eventSender_location = location;
> +        _eventSender_locationInWindow = globalLocation; //????????
> +        _eventSender_stage = stage;
> +        _eventSender_pressure = pressure;
> +        _eventSender_phase = phase;
> +        _eventSender_timestamp = time;
> +        _eventSender_eventNumber = eventNumber;
> +    }
> +
> +    return self;

I know this isn't the general ObjC convention, but I was looking at Safari code yesterday that did this the other way around:

if (!self)
  return nil;

// other setup stuff

I guess we don't have coding style for that?

[Beth Dakin]

(In reply to comment #13)
> Comment on attachment 250904 [details]
> Patch with tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250904&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2124
> > +    bool swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforcechangedEvent, mouseEvent.targetNode(), false, 0, event, false);
> > +    if (event.type() == PlatformEvent::MouseForceDown)
> > +        swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforcedownEvent, mouseEvent.targetNode(), false, 0, event, false);
> > +    if (event.type() == PlatformEvent::MouseForceUp)
> > +        swallowedEvent = !dispatchMouseEvent(eventNames().webkitmouseforceupEvent, mouseEvent.targetNode(), false, 0, event, false);
> 
> Does this mean we can only swallow the event from forcechanged if down/up
> also swallow?
> 
> Technically I guess you could return from within both conditionals.
> 

I'm still not 100% on the right thing to do here, but I changed it to |=, which I think is right.

> > Source/WebCore/replay/UserInputBridge.cpp:163
> > +bool UserInputBridge::handleMouseForceChangedEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> > +{
> > +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> > +}
> > +
> > +bool UserInputBridge::handleMouseForceDownEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> > +{
> > +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> > +}
> > +
> > +bool UserInputBridge::handleMouseForceUpEvent(const PlatformMouseEvent& mouseEvent, InputSource)
> > +{
> > +    return m_page.mainFrame().eventHandler().handleMouseForceEvent(mouseEvent);
> > +}
> 
> I haven't looked at how these are called, but is there any reason why they
> can't all be done in one handleMouseForceEvent? Do we need one for each type
> of input event name?
> 

Fixed this!

> > Source/WebKit2/Shared/mac/WebEventFactory.mm:384
> > +        if (lastPressureEvent.stage == 1 && event.stage == 2)
> > +            type = WebEvent::MouseForceDown;
> > +        else if (lastPressureEvent.stage == 2 && event.stage == 1)
> > +            type = WebEvent::MouseForceUp;
> > +        else
> 
> This is probably silly, but I wonder if we should have enums for the stage
> values so they read a bit better. Or are they actually just a count?
> 

I don't think it's silly at all because it is very confusing! But since the use of stage is limited to this one function at this time, I don't think it's super worthwhile. If we end up needing to use the stage in more places or propagate it down to WebCore, we should absolutely use an enum.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1912
> > +        case PlatformEvent::MouseForceChanged:
> > +            return page->corePage()->userInputBridge().handleMouseForceChangedEvent(platformMouseEvent);
> > +        case PlatformEvent::MouseForceDown:
> > +            return page->corePage()->userInputBridge().handleMouseForceDownEvent(platformMouseEvent);
> > +        case PlatformEvent::MouseForceUp:
> > +            return page->corePage()->userInputBridge().handleMouseForceUpEvent(platformMouseEvent);
> 
> Ooooh. Here's my answer to the other bit. So, considering these all do the
> same thing on the other end, do you think it makes sense to have a single
> handle function?
> 

Yes, good call. Done.

> > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:73
> > +    self = [super init];
> > +
> > +    if (self) {
> > +        _eventSender_location = location;
> > +        _eventSender_locationInWindow = globalLocation; //????????
> > +        _eventSender_stage = stage;
> > +        _eventSender_pressure = pressure;
> > +        _eventSender_phase = phase;
> > +        _eventSender_timestamp = time;
> > +        _eventSender_eventNumber = eventNumber;
> > +    }
> > +
> > +    return self;
> 
> I know this isn't the general ObjC convention, but I was looking at Safari
> code yesterday that did this the other way around:
> 
> if (!self)
>   return nil;
> 
> // other setup stuff
> 
> I guess we don't have coding style for that?


Sounds nicer to me! I switched to this format.

I also added releases to the NSEvents for smfr.

[Beth Dakin]

Created attachment 250949 [details]
Patch for bots

[Beth Dakin]

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

Thanks Dean!

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143881

[Alexey Proskuryakov]

Rolling out for now, details in e-mail.

[Beth Dakin]

Thanks Alexey. Giving this another go with http://trac.webkit.org/changeset/182963