Bug 93125 – [Qt][WK2] Remove workarounds from input event handling

• Component: New Bugs
• Status: RESOLVED FIXED

[Andras Becsi]

[Qt][WK2] Remove workarounds from input event handling

[Andras Becsi]

Created attachment 156385 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 156385 [details]
Patch

Looks ok to me, maybe jocelyn wants to have a look

[Jocelyn Turcotte]

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?

[Andras Becsi]

(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.

[Andras Becsi]

Created attachment 156660 [details]
Patch

[Jocelyn Turcotte]

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.

[Andras Becsi]

(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.

[Andras Becsi]

Comment on attachment 156660 [details]
Patch

Clearing flags on attachment: 156660

Committed r124758: <http://trac.webkit.org/changeset/124758>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.