[Qt][WK2] Click, mouse and links rely on touch mocking

[Noam Rosenthal]

When loading an HTML file from QML without MiniBrowser, in "mobile" mode, links and mouse-events don't work.
Those start to work again when using touch-mocking, or when disabling Flickable.

This makes the mobile-version of QQuickWebView unusable without MiniBrowser.

[Noam Rosenthal]

Created attachment 135448 [details]
QML loader

QML file showing the problem

[Noam Rosenthal]

Created attachment 135449 [details]
HTML file with some mouse/links.

[Noam Rosenthal]

https://bugs.webkit.org/show_bug.cgi?id=83033 Seems to fix this.

[Andras Becsi]

(In reply to comment #0)
> When loading an HTML file from QML without MiniBrowser, in "mobile" mode, links and mouse-events don't work.
> Those start to work again when using touch-mocking, or when disabling Flickable.

Subclassing from flickable should not change the behaviour for links so I wonder why it fixed your issue.

In "mobile" mode on a device which has only touch events clicking on links happens with tap gestures.
If this does not work with touch events then there is something wrong with the tap gesture recognizer.

The mouse events are disabled for the flickable webview for now, since it would select text when panning, but after http://codereview.qt-project.org/21896 we can enable it again.

[Noam Rosenthal]

> Subclassing from flickable should not change the behaviour for links so I wonder why it fixed your issue.

It didn't, I tested again. Might have been a testing error.

> In "mobile" mode on a device which has only touch events clicking on links happens with tap gestures.

There's no way to disable mobile mode

> If this does not work with touch events then there is something wrong with the tap gesture recognizer.
> 
> The mouse events are disabled for the flickable webview for now, since it would select text when panning, but after http://codereview.qt-project.org/21896 we can enable it again.

So, right now, there's still no way to get pages with links running with pure QML... Let's see if that gets fixed after that Qt bug is in.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #4)
> The mouse events are disabled for the flickable webview for now, since it would select text when panning, but after http://codereview.qt-project.org/21896 we can enable it again.

For the record: the mentioned change was abandoned in favor of another one https://codereview.qt-project.org/#change,24189

[Andras Becsi]

Created attachment 155290 [details]
proposed patch

[Andras Becsi]

(In reply to comment #7)
> Created an attachment (id=155290) [details]
> proposed patch

Simon, is this similar to what you had in mind when we were discussing this issue?

[Simon Hausmann]

Comment on attachment 155290 [details]
proposed patch

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

Looks good in general, a couple of small issues and questions :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:460
> +        isMouseEvent = true;
>      case QEvent::TouchBegin:

I think between those two lines we usually add a "// Fall through" comment, to indicate that the "fall through" is deliberate.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:463
> +        m_isMouseButtonPressed = true;

Shouldn't this be set in QEvent::MouseButtonPress?

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:476
> +        isMouseEvent = true;
>      case QEvent::TouchUpdate:

Fall through comment.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:483
> +        isMouseEvent = true;
>      case QEvent::TouchEnd:

And here :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:484
> +        m_isMouseButtonPressed = false;

Same question as above: Should this be in case QEvent::MouseButtonRelease?

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:515
> +        currentTouchPoint.setId(mouseEvent->buttons());

Maybe worth a comment, it's easy to overlook this trick when glancing through the code :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:517
> +        currentTouchPoint.setRect(QRectF(mouseEvent->localPos(), QSizeF(40, 40)));

Hm, why 40, 40 instead of 1, 1?

[Andras Becsi]

Created attachment 156077 [details]
proposed patch

[Simon Hausmann]

Comment on attachment 156077 [details]
proposed patch

r=me nice :)

[Andras Becsi]

Comment on attachment 156077 [details]
proposed patch

Clearing flags on attachment: 156077

Committed r124455: <http://trac.webkit.org/changeset/124455>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.