Bug 84350 – [Qt][WK2] Implement axis locking on the WebView for pan gestures

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

If a pan gesture has sufficient velocity along one axis the WebView should automatically lock the page movement to that axis. This locking should be maintained until the ongoing pan gesture ends.

[Andras Becsi]

Created attachment 137916 [details]
proposed patch

[zalan]

Comment on attachment 137916 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:128
> +    m_state = Initial;

I'd reset m_sampleCount here and not in ::update()

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:134
> +        if (!viewport->isDraggingHorizontally()) {

Can't we make this decision in the state machine at ::update() and not go into locked mode at all, if the viewport is not pannable in that direction? Though I don't even know how we can end up in horizontallyLocked state, if the viewport is not pannable in that direction. I must have overlooked something here.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:147
> +

It feels like there's one too many state here. Like if the 'if (m_state == Initial) {' part could be removed from ::update, (by defining a proper starting point for this functionality) then undefined would not be needed at all. ::update would be sampling until state goes to something other than Initial. Not a big deal, just thinking it could be a little simpler. (provided that making these change gets the logic actually simpler)

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:186
>      QDeclarativeComponent* promptDialog;

Great feature btw.

[Kenneth Rohde Christiansen]

Comment on attachment 137916 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:60
> +static const qreal kAxisLockVelocityThreshold = 100.;

remove the .

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:105
> +    static bool touchVelocityAvailable = event->device()->capabilities().testFlag(QTouchDevice::Velocity);
> +    QVector2D velocity;
> +
> +    if (touchVelocityAvailable)
> +        velocity = touchPoint.velocity();
> +    else {
> +        const QLineF movementLine(touchPoint.screenPos(), m_initialScreenPosition);
> +        const qint64 elapsed = m_time.elapsed();
> +
> +        if (!elapsed)
> +            return;
> +
> +        // Calculate an approiximate velocity vector in the unit of pixel / second.
> +        velocity = QVector2D(1000 * movementLine.dx() / elapsed, 1000 * movementLine.dy() / elapsed);

Separate this out in a method like QVector2D touchVelocity(event) or so...?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:121
> +void QQuickWebViewPrivate::AxisLocker::setInitialPoint(const QPointF& position)

setInitialPosition?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1509
> +    Q_D(QQuickWebView);
> +    QMouseEvent mouseEvent(QEvent::MouseMove, d->axisLocker.adjust(position), Qt::LeftButton, Qt::NoButton, Qt::NoModifier);

can't adjust and update be merged so that the handle* methods already gets called with the right position? That would seem more logical.

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 137916 [details])
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:134
> > +        if (!viewport->isDraggingHorizontally()) {
> 
> Can't we make this decision in the state machine at ::update() and not go into locked mode at all, if the viewport is not pannable in that direction? Though I don't even know how we can end up in horizontallyLocked state, if the viewport is not pannable in that direction. I must have overlooked something here.

We can do the check earlier in touchEvent(), which makes it simpler.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:147
> > +
> 
> It feels like there's one too many state here. Like if the 'if (m_state == Initial) {' part could be removed from ::update, (by defining a proper starting point for this functionality) then undefined would not be needed at all. ::update would be sampling until state goes to something other than Initial. Not a big deal, just thinking it could be a little simpler. (provided that making these change gets the logic actually simpler)

We could decide if the current event could be an initial event for a pan gesture in the touchEvent() method.
But since the start of a pan gesture can also happen in a transition from a pinch gesture it would need more complex logic to decide when to set the initial screen position than to have an additional state in the state machine.

[Andras Becsi]

(In reply to comment #3)
> (From update of attachment 137916 [details])
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1509
> > +    Q_D(QQuickWebView);
> > +    QMouseEvent mouseEvent(QEvent::MouseMove, d->axisLocker.adjust(position), Qt::LeftButton, Qt::NoButton, Qt::NoModifier);
> 
> can't adjust and update be merged so that the handle* methods already gets called with the right position? That would seem more logical.

The point of having the separation is to be able to decide the locking prior to actually recognizing the pan gesture (which happens when the event comes back from the web process).

This in practice means that we collect samples in the first entry point of the touch event (in QQuickWebView::touchEvent), and adjust the position when the gesture recognizer sends the actual gesture position through the interaction engine to the flickable.
These two actions happen asynchronously, which means the incoming event might have arbitrary delays until it is actually recognized as a pan gesture.

[Andras Becsi]

Created attachment 138065 [details]
proposed patch v2

[zalan]

Comment on attachment 138065 [details]
proposed patch v2

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

Looks great.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:121
> +            m_state = VerticallyLocked;

It's really just nitpicking and matter of habit (and should have mentioned it in the first round), but i'd do m_state = directionIndicator > 0 ? HorizontallyLocked : VerticallyLocked; just to make it a oneliner. Ignore it if you want.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1385
> +                          && height() < contentHeight();

I hope these content* and width()/height() functions aren't too expensive to check them on each touch event.

[Andras Becsi]

(In reply to comment #7)
> (From update of attachment 138065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138065&action=review
> 
> Looks great.
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:121
> > +            m_state = VerticallyLocked;
> 
> It's really just nitpicking and matter of habit (and should have mentioned it in the first round), but i'd do m_state = directionIndicator > 0 ? HorizontallyLocked : VerticallyLocked; just to make it a oneliner. Ignore it if you want.

Sure it can be a one liner. It remained like this for historical reasons, since I had additional logic there.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1385
> > +                          && height() < contentHeight();
> 
> I hope these content* and width()/height() functions aren't too expensive to check them on each touch event.

That is a good point.
Although I do not think they are that expensive, but this new logic is definitely more expensive than the previous approach to just check the relevant viewport->isDragging*() methods in the ::adjust function, as I did in my initial patch.

I think I'm going to revert that part of the change.

[zalan]

> That is a good point.
> Although I do not think they are that expensive, but this new logic is definitely more expensive than the previous approach to just check the relevant viewport->isDragging*() methods in the ::adjust function, as I did in my initial patch.
> 
> I think I'm going to revert that part of the change.
If the page is not pannable in one direction, does it make sense to even start sampling for axis locking? Wouldn't it be automatically locked in in one direction in that case?

[Andras Becsi]

(In reply to comment #9)
> > That is a good point.
> > Although I do not think they are that expensive, but this new logic is definitely more expensive than the previous approach to just check the relevant viewport->isDragging*() methods in the ::adjust function, as I did in my initial patch.
> > 
> > I think I'm going to revert that part of the change.
> If the page is not pannable in one direction, does it make sense to even start sampling for axis locking? Wouldn't it be automatically locked in in one direction in that case?

Yes, it would be locked by the Flickable if the policy is the default AutoFlickDirection.
Sampling would indeed not make sense in this default scenario.

What I realized now is the question what we do for HorizontalAndVerticalFlick.
I think it should disable axis locking.

[Andras Becsi]

Created attachment 138074 [details]
proposed patch

[Andras Becsi]

Created attachment 138075 [details]
updated patch

Add a missing hunk.

[Kenneth Rohde Christiansen]

Comment on attachment 138075 [details]
updated patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:70
> +QQuickWebViewPrivate::AxisLocker::AxisLocker()

FlickableAxisLocker?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:140
> +        enum LockingState {

Wouldn't a flag be better instead? AllowedPanningDirection? 

You already know whether you are sampling or not due to the counter

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:143
> +            NotLocked,

Unlocked?

[Andras Becsi]

Created attachment 138124 [details]
proposed patch

Addressed comments and further tweaked the constraints to behave better on the device.

[zalan]

Comment on attachment 138124 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:117
> +}

the first update call when the sampling starts should early return as there's no value in calculating velocity at this point. (as it was done previously) so either move ++m_sampleCount up and check against value 1 or do something like this if (!m_sampleCount++), though i dont really like this as it's highly error prone. I dont have a strong preference on this.

[Andras Becsi]

(In reply to comment #15)
> (From update of attachment 138124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138124&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:117
> > +}
> 
> the first update call when the sampling starts should early return as there's no value in calculating velocity at this point. (as it was done previously) so either move ++m_sampleCount up and check against value 1 or do something like this if (!m_sampleCount++), though i dont really like this as it's highly error prone. I dont have a strong preference on this.

D'oh, missing hunk, I should learn to properly squash my patches...
Can fix that when landing.

[zalan]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 138124 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138124&action=review
> > 
> > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:117
> > > +}
> > 
> > the first update call when the sampling starts should early return as there's no value in calculating velocity at this point. (as it was done previously) so either move ++m_sampleCount up and check against value 1 or do something like this if (!m_sampleCount++), though i dont really like this as it's highly error prone. I dont have a strong preference on this.
> 
> D'oh, missing hunk, I should learn to properly squash my patches...
> Can fix that when landing.
:) ok. lgtm otherwise.

[Andras Becsi]

Created attachment 138141 [details]
patch for review

[Kenneth Rohde Christiansen]

Comment on attachment 138141 [details]
patch for review

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:140
> +        enum AllowedPanningDirection {

This could be enum AllowedPanningDirecton { Horizontally = 0x01, Vertically = 0x02 }

So you can do if (allowedDirection & Horizontally) { }

[Andras Becsi]

(In reply to comment #19)
> (From update of attachment 138141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138141&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:140
> > +        enum AllowedPanningDirection {
> 
> This could be enum AllowedPanningDirecton { Horizontally = 0x01, Vertically = 0x02 }
> 
> So you can do if (allowedDirection & Horizontally) { }

Well, I do not see how this would improve the patch.
In my opinion it would only make sense if the axis locker would also have a state which would be a combination of the flags, which it does not since the unlocked state is _not_ a combination of horizontally locked and vertically locked (this would mean that the page can not be moved at all).

Additionally: horizontally and vertically are adverbs which would need a verb like locked or panning to be meaningful.

Thus I think this would just make the state machine misleading and over-engineered.

[Andras Becsi]

What I meant to point out was that the axis locker locks the panning direction either in vertical or in horizontal direction. This is the premise of my argument.

These two states are mutually exclusive, which implies that there is a third state in which the direction is not locked in either direction. The additional undefined state is only needed for the desktop (which does not supply valid touch velocity) to initialize the velocity calculation.

So along these lines if we have flags which can be combined, the combination could logically either mean that panning is only allowed along the x and y axes but not diagonally, or that the movement is locked to both axes, thus panning is completely disabled, but _not_ that the page is freely movable.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> What I meant to point out was that the axis locker locks the panning direction either in vertical or in horizontal direction. This is the premise of my argument.
> 
> These two states are mutually exclusive, which implies that there is a third state in which the direction is not locked in either direction. The additional undefined state is only needed for the desktop (which does not supply valid touch velocity) to initialize the velocity calculation.
> 
> So along these lines if we have flags which can be combined, the combination could logically either mean that panning is only allowed along the x and y axes but not diagonally, or that the movement is locked to both axes, thus panning is completely disabled, but _not_ that the page is freely movable.

huh? If both directions (they are flags, and can be combined) are set you can pan in both directions. So when you lock, you simple disallow panning in a specific direction.

[zalan]

(In reply to comment #22)
> (In reply to comment #21)
> > What I meant to point out was that the axis locker locks the panning direction either in vertical or in horizontal direction. This is the premise of my argument.
> > 
> > These two states are mutually exclusive, which implies that there is a third state in which the direction is not locked in either direction. The additional undefined state is only needed for the desktop (which does not supply valid touch velocity) to initialize the velocity calculation.
> > 
> > So along these lines if we have flags which can be combined, the combination could logically either mean that panning is only allowed along the x and y axes but not diagonally, or that the movement is locked to both axes, thus panning is completely disabled, but _not_ that the page is freely movable.
> 
> huh? If both directions (they are flags, and can be combined) are set you can pan in both directions. So when you lock, you simple disallow panning in a specific direction.

I guess, the confusion started when the enum was renamed, while values got unchanged. AllowedPanningDirection now assumes directions where panning is possible, while they are really values of directions where panning is _not_ possible. I like Kenneth's proposal of moving from the banning perspective to the allowing.

[Andras Becsi]

(In reply to comment #23)
> I guess, the confusion started when the enum was renamed, while values got unchanged. AllowedPanningDirection now assumes directions where panning is possible, while they are really values of directions where panning is _not_ possible. I like Kenneth's proposal of moving from the banning perspective to the allowing.

Agreed, the AllowedPanningDirection name is really confusing now. I'm going to fix this, although the banning perspective seems more logical to me since we are talking about an axis locker, and not a panning enabler.
But let's have a positive perspective :)

[zalan]

(In reply to comment #24)
> (In reply to comment #23)
> > I guess, the confusion started when the enum was renamed, while values got unchanged. AllowedPanningDirection now assumes directions where panning is possible, while they are really values of directions where panning is _not_ possible. I like Kenneth's proposal of moving from the banning perspective to the allowing.
> 
> Agreed, the AllowedPanningDirection name is really confusing now. I'm going to fix this, although the banning perspective seems more logical to me since we are talking about an axis locker, and not a panning enabler.
> But let's have a positive perspective :)
:) yeah, agree. Do the Kenneth way and be permissive.

[Andras Becsi]

Created attachment 138321 [details]
updated patch

[Kenneth Rohde Christiansen]

Comment on attachment 138321 [details]
updated patch

Nice!

[Andras Becsi]

Comment on attachment 138321 [details]
updated patch

Clearing flags on attachment: 138321

Committed r115082: <http://trac.webkit.org/changeset/115082>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.