Bug 32432 - [Qt] Add support for touch events in QWebView and QGraphicsWebView
Summary: [Qt] Add support for touch events in QWebView and QGraphicsWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 32434 32485
  Show dependency treegraph
 
Reported: 2009-12-11 08:01 PST by Simon Hausmann
Modified: 2009-12-17 09:09 PST (History)
4 users (show)

See Also:


Attachments
Patch for adding support for touch events in QWebView and QGraphicsWebView. (2.63 KB, patch)
2009-12-15 04:53 PST, Kim Grönholm
hausmann: review-
Details | Formatted Diff | Diff
Updated patch according to Simon's comments (2.73 KB, patch)
2009-12-15 13:57 PST, Kim Grönholm
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2009-12-11 08:01:43 PST
QWebPage has been extended with support for handling QTouchEvents and forwarding them to the event handler.

However what is still missing is

    1) Forwarding touch events from QGraphicsWebView and QWebView to QWebPage

    2) Enabling the WA_AcceptsTouch widget attribute


It may seem like we would have to dynamically toggle the WA_AcceptsTouchEvents widget attribute, but since in the long run we want to have have QWebPage also interpret touch events (for example for panning), we might as well enable it all the time ;-)
Comment 1 Kim Grönholm 2009-12-15 04:53:14 PST
Created attachment 44866 [details]
Patch for adding support for touch events in QWebView and QGraphicsWebView.
Comment 2 WebKit Review Bot 2009-12-15 04:55:12 PST
style-queue ran check-webkit-style on attachment 44866 [details] without any errors.
Comment 3 Simon Hausmann 2009-12-15 06:20:27 PST
Comment on attachment 44866 [details]
Patch for adding support for touch events in QWebView and QGraphicsWebView.

The patch looks good to me! Just a few minor style nitpicks that need to be fixed before we can land this:

> +
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> +    if (d->page) {
> +        if (event->type() == QEvent::TouchBegin 
> +            || event->type() == QEvent::TouchEnd 
> +            || event->type() == QEvent::TouchUpdate) {
> +                d->page->event(event);
> +        }

The curly braces are left out when the body is only a one-liner.

I would recommend combining the two if statements:

if (d->page && event->type() == ...
    && ... )
    d->page->event(event);

Also, with the above code we always call QWidget::event() with the touch event, which will
convert the first touch to a mouse event (according to the docs).

I think perhaps the code should be like:

if (...) {
    d->page->event(event);
    if (event->isAccepted())
        return true;
}

...
else we call QWidget::event.

The same for QGraphicsWebView and QWebView of course.
Comment 4 Kim Grönholm 2009-12-15 13:57:59 PST
Created attachment 44908 [details]
Updated patch according to Simon's comments
Comment 5 WebKit Review Bot 2009-12-15 14:02:33 PST
style-queue ran check-webkit-style on attachment 44908 [details] without any errors.
Comment 6 Simon Hausmann 2009-12-17 08:58:29 PST
Comment on attachment 44908 [details]
Updated patch according to Simon's comments

r=me
Comment 7 Benjamin Poulain 2009-12-17 09:04:36 PST
Comment on attachment 44908 [details]
Updated patch according to Simon's comments

>+    if (d->page && (event->type() == QEvent::TouchBegin
>+                || event->type() == QEvent::TouchEnd
>+                || event->type() == QEvent::TouchUpdate)) {

I think the indentation is not correct here.

Looks good otherwise.
Comment 8 WebKit Commit Bot 2009-12-17 09:09:33 PST
Comment on attachment 44908 [details]
Updated patch according to Simon's comments

Clearing flags on attachment: 44908

Committed r52256: <http://trac.webkit.org/changeset/52256>
Comment 9 WebKit Commit Bot 2009-12-17 09:09:40 PST
All reviewed patches have been landed.  Closing bug.