Bug 92394 – [Qt] Fix axis locking when panning on N9

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Lauro Moura Maranhao Neto]

Steps to reproduce:

- Run a rather recent version of Qt5 and WebView (Since June), opening up a page that allows panning in all directions (e.g. a zoomed page).
- Pan it.
- Lift the finger, the screen will update (which currently is slow, taking at least at least a couple of seconds, but it's another problem).
- While the screen is updating, slowly swipe the finger again to pan, or even just press the screen.
- Almost always the screen will lock. If panning vertically, the screen will lock horizontally.

About locking easily, the problem seems to be that the AxisLocker class uses a QElapsedTimer to check the time between the events. The intense CPU work when updating the screen causes the events to be delivered late, leading to wrong elapsed time values. Often the touch events were received with less than 5 miliseconds between them.

About the wrong axis, on N9 the native X orientation is landscape. So when swiping vertically in portrait mode (default usage), the TouchPoing.screenPos() coordinates changes the x member, not the expected y member.

Cleaning up a working patch to upload.

[Lauro Moura Maranhao Neto]

Created attachment 154714 [details]
Patch

[Andras Becsi]

Comment on attachment 154714 [details]
Patch

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

Switching to the timestamp to calculate the elapsed time is a good idea.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:227
> +    if (event->type() == QEvent::TouchBegin) {
> +        const QTouchEvent::TouchPoint& initialPoint = event->touchPoints().first();
> +        // Use screenPos as it'll be used to create synthetic MouseEvents
> +        setReferencePosition(initialPoint.screenPos());
> +        m_initialPosition = initialPoint.pos();
> +        m_time = event->timestamp();
> +        m_sampleCount = 0;
> +        return;
> +    }
>  
> -    if (m_sampleCount == 1) {
> -        m_initialScreenPosition = touchPoint.screenPos();
> -        m_time.restart();
> +    if (event->type() == QEvent::TouchEnd) {
> +        reset();
>          return;
>      }

Depending on TouchBegin and TouchEnd for axis locking does not seem right to me since a pan gesture might start with a TouchUpdate which has only one touch point (the previous update had multiple points -> transition from a pinch gesture) and it might end with a TouchUpdate when the event has multiple touch points (transition to a potential pinch gesture).

[Lauro Moura Maranhao Neto]

(In reply to comment #2)
> (From update of attachment 154714 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154714&action=review
> Depending on TouchBegin and TouchEnd for axis locking does not seem right to me since a pan gesture might start with a TouchUpdate which has only one touch point (the previous update had multiple points -> transition from a pinch gesture) and it might end with a TouchUpdate when the event has multiple touch points (transition to a potential pinch gesture).

Good point. Going to update the patch.

[Lauro Moura Maranhao Neto]

Created attachment 155079 [details]
Patch

[Kenneth Rohde Christiansen]

Andras what do you think about this? Btw, remember adding punctuation mark to the end of comments

[Andras Becsi]

Comment on attachment 155079 [details]
Patch

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

The patch looks better now but I still have some concerns. It would also be good to have a manual test for this in ManualTests/qt if possible, which triggers the issues on the N9.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:219
> +        // Use screenPos as it'll be used to create synthetic MouseEvents
> +        setReferencePosition(initialPoint.screenPos());

I do not really understand this part. Why do we need the screenPos for the reference position when the pan gesture recognizer uses touchPoint.pos() when calling panGestureRequestUpdate which position is then used for the MouseEvent? The incoming touch position is in viewport coordinates and is the same local position which is then used by the pan gesture recognizer to notify the Flickable. With this change we end up with one component of the position in local coordinates and the other in screen coordinates when locking.
I might miss something but the issue you try to fix here seems rather to be an orientation bug on the N9 where the touch coordinates do not get properly translated to portrait mode in the canvas.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1874
> -    Q_D(QQuickWebView);
> -    d->axisLocker.setReferencePosition(position);

The intention of setting the reference point when the notification comes from the pan gesture recognizer (after coming back from the web process) was that some events might be handled by the page and in that case the event is ignored by the event handler. So if the reference position is set on the incoming side we might end up with a wrong reference position in cases when that event is ignored but subsequent events get through to the Flickable without triggering a reset of the lock. Though it might not be possible to produce a page that triggers errors related to this behaviour.

[Lauro Moura Maranhao Neto]

Created attachment 155546 [details]
Patch

Updated the patch removing the setReferencePosition changes. Now it only uses pos() and the timestamp to calculate the local velocity. Indeed on N9 the screenPos is locked to landscape, with 0,0 being the top right corner in portrait mode. But wouldn't just using the local position be enough to get the swipe velocity calculation right?

[Andras Becsi]

Comment on attachment 155546 [details]
Patch

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

It should be OK to use the local position for the velocity calculation.
This looks good to me now.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:204
> +    const ulong elapsed = event->timestamp() - m_time;
>  
>      if (!elapsed)
>          return QVector2D(0, 0);

You could add an ASSERT to assure that the calculated elapsed time is positive, but that's only a nitpick.

[Lauro Moura Maranhao Neto]

(In reply to comment #8)
> (From update of attachment 155546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155546&action=review
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:204
> > +    const ulong elapsed = event->timestamp() - m_time;
> >  
> >      if (!elapsed)
> >          return QVector2D(0, 0);
> 
> You could add an ASSERT to assure that the calculated elapsed time is positive, but that's only a nitpick.

Both timestamp() and m_time are ulongs, so it's pretty safe to just subtract.

[Andras Becsi]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 155546 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155546&action=review
> > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:204
> > > +    const ulong elapsed = event->timestamp() - m_time;
> > >  
> > >      if (!elapsed)
> > >          return QVector2D(0, 0);
> > 
> > You could add an ASSERT to assure that the calculated elapsed time is positive, but that's only a nitpick.
> 
> Both timestamp() and m_time are ulongs, so it's pretty safe to just subtract.
Unsigned indeed, disregard my rubbish. It's just my paranoia.

[Simon Hausmann]

Comment on attachment 155546 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:145
> +        ulong m_time;

Shouldn't the constructor try to initialize this variable to reduce the risk of accessing it uninitialized?

[Lauro Moura Maranhao Neto]

Created attachment 156085 [details]
Patch

Updated the patch initializing m_time.

[WebKit Review Bot]

Comment on attachment 156085 [details]
Patch

Clearing flags on attachment: 156085

Committed r124462: <http://trac.webkit.org/changeset/124462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.