[Qt][WK2] Remove workarounds from input event handling
Created attachment 156385 [details] Patch
Comment on attachment 156385 [details] Patch Looks ok to me, maybe jocelyn wants to have a look
Comment on attachment 156385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156385&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1672 > - if (!isVisible() || !isEnabled() || !s_flickableViewportEnabled) > + if (!isVisible() || !isEnabled()) Humm weird, feels like this was done the wrong way, that when !s_flickableViewportEnabled we should return false and not delegate to flickable. Why did you change it by the way? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1696 > + // Force mouse events targeting the web page through the default > + // propagation path of mouse*Event(). This way the events meant > + // for a child dialog are also properly propagated to the dialog. > + if (page()->contains(localPos)) > + return false; Could you give an example case of when this is needed? It feels to me that any item with the page in the content item should behave the same regarding mouse events, no? Plus what would happen if a dialog is on top of the page, but slightly offset so that half of it is over the page, and half outside. Mouse events on the outer half could get eaten by flickable but not when in the half "contained" by the page under. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1702 > case QEvent::TouchBegin: > case QEvent::TouchUpdate: > case QEvent::TouchEnd: QQuickFlickable::childMouseEventFilter seems to return false for all touch events, not sure if this changed recently. If it changed, do you think we still need this?
(In reply to comment #3) > (From update of attachment 156385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156385&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1672 > > - if (!isVisible() || !isEnabled() || !s_flickableViewportEnabled) > > + if (!isVisible() || !isEnabled()) > > Humm weird, feels like this was done the wrong way, that when !s_flickableViewportEnabled we should return false and not delegate to flickable. Why did you change it by the way? This was a workaround so that the behaviour for the desktop webview does not change with the reimplemented function. As far as I remember returning false made some other tests fail for me locally but theoretically it shouldn't make a difference for the desktop webview. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1696 > > + // Force mouse events targeting the web page through the default > > + // propagation path of mouse*Event(). This way the events meant > > + // for a child dialog are also properly propagated to the dialog. > > + if (page()->contains(localPos)) > > + return false; > > Could you give an example case of when this is needed? It feels to me that any item with the page in the content item should behave the same regarding mouse events, no? > This is needed if there is another item, let's say a mouse area on the contentItem besides the page. If the event is targeting that additional item it should be checked by the Flickable if it is part of a potential flick before it reaches the item. But since we do not support this anyway I'm going to remove it and also remove the experimental useDefaultContentSize property which would only be used for this kind of usecase. > Plus what would happen if a dialog is on top of the page, but slightly offset so that half of it is over the page, and half outside. Mouse events on the outer half could get eaten by flickable but not when in the half "contained" by the page under. Flickable does a similar hit-test for mouse events so in this case it would not grab the event. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1702 > > case QEvent::TouchBegin: > > case QEvent::TouchUpdate: > > case QEvent::TouchEnd: > > QQuickFlickable::childMouseEventFilter seems to return false for all touch events, not sure if this changed recently. If it changed, do you think we still need this? You are right, Flickable does not handle touch events, I just wanted to prevent relying on this fact. But this is just my paranoia. I'm going to update the patch on Monday.
Created attachment 156660 [details] Patch
Comment on attachment 156660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156660&action=review Just one nitpick. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1694 > + ASSERT(event->type() == QEvent::UngrabMouse); Maybe I'm not seeing well its purpose but it feels like this might backfire more often that it would help us. I'm also just a bit tired of asserts popping in the UI code that you can just comment out without it exposing any real bug.
(In reply to comment #6) > (From update of attachment 156660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156660&action=review > > Just one nitpick. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1694 > > + ASSERT(event->type() == QEvent::UngrabMouse); > > Maybe I'm not seeing well its purpose but it feels like this might backfire more often that it would help us. I'm also just a bit tired of asserts popping in the UI code that you can just comment out without it exposing any real bug. If we hit this assert it means that something is going terribly wrong with the childMouseEventFilter function. I find it already somewhat counter-intuitive that it is also used for touch events and if the currently assumed behaviour changes we might have to adjust our code again. Else it could bypass unnoticed and may cause bugs that are hard to find.
Comment on attachment 156660 [details] Patch Clearing flags on attachment: 156660 Committed r124758: <http://trac.webkit.org/changeset/124758>
All reviewed patches have been landed. Closing bug.