Bug 84350 - [Qt][WK2] Implement axis locking on the WebView for pan gestures
: [Qt][WK2] Implement axis locking on the WebView for pan gestures
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: All Linux
: P3 Normal
Assigned To:
:
: Qt
:
: 76773
  Show dependency treegraph
 
Reported: 2012-04-19 09:57 PST by
Modified: 2012-04-24 10:48 PST (History)


Attachments
proposed patch (9.56 KB, patch)
2012-04-19 10:17 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch v2 (9.66 KB, patch)
2012-04-20 03:14 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (9.40 KB, patch)
2012-04-20 05:31 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (9.43 KB, patch)
2012-04-20 05:46 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (9.45 KB, patch)
2012-04-20 11:19 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
patch for review (9.47 KB, patch)
2012-04-20 12:15 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (9.54 KB, patch)
2012-04-23 03:35 PST, Andras Becsi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-19 09:57:02 PST
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.
------- Comment #1 From 2012-04-19 10:17:22 PST -------
Created an attachment (id=137916) [details]
proposed patch
------- Comment #2 From 2012-04-19 11:27:25 PST -------
(From update of attachment 137916 [details])
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.
------- Comment #3 From 2012-04-19 16:28:21 PST -------
(From update of attachment 137916 [details])
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.
------- Comment #4 From 2012-04-20 03:02:29 PST -------
(In reply to comment #2)
> (From update of attachment 137916 [details] [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.
------- Comment #5 From 2012-04-20 03:13:14 PST -------
(In reply to comment #3)
> (From update of attachment 137916 [details] [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.
------- Comment #6 From 2012-04-20 03:14:09 PST -------
Created an attachment (id=138065) [details]
proposed patch v2
------- Comment #7 From 2012-04-20 03:40:29 PST -------
(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.

> 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.
------- Comment #8 From 2012-04-20 04:34:56 PST -------
(In reply to comment #7)
> (From update of attachment 138065 [details] [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.
------- Comment #9 From 2012-04-20 04:52:08 PST -------
> 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?
------- Comment #10 From 2012-04-20 05:18:19 PST -------
(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.
------- Comment #11 From 2012-04-20 05:31:03 PST -------
Created an attachment (id=138074) [details]
proposed patch
------- Comment #12 From 2012-04-20 05:46:46 PST -------
Created an attachment (id=138075) [details]
updated patch

Add a missing hunk.
------- Comment #13 From 2012-04-20 09:36:32 PST -------
(From update of attachment 138075 [details])
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?
------- Comment #14 From 2012-04-20 11:19:49 PST -------
Created an attachment (id=138124) [details]
proposed patch

Addressed comments and further tweaked the constraints to behave better on the device.
------- Comment #15 From 2012-04-20 11:35:12 PST -------
(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.
------- Comment #16 From 2012-04-20 11:43:22 PST -------
(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.
------- Comment #17 From 2012-04-20 11:50:54 PST -------
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 138124 [details] [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.
------- Comment #18 From 2012-04-20 12:15:26 PST -------
Created an attachment (id=138141) [details]
patch for review
------- Comment #19 From 2012-04-21 09:48:05 PST -------
(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) { }
------- Comment #20 From 2012-04-21 12:10:01 PST -------
(In reply to comment #19)
> (From update of attachment 138141 [details] [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.
------- Comment #21 From 2012-04-21 14:28:27 PST -------
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.
------- Comment #22 From 2012-04-22 09:04:03 PST -------
(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.
------- Comment #23 From 2012-04-22 10:00:40 PST -------
(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.
------- Comment #24 From 2012-04-22 10:34:10 PST -------
(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 :)
------- Comment #25 From 2012-04-22 10:45:16 PST -------
(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.
------- Comment #26 From 2012-04-23 03:35:54 PST -------
Created an attachment (id=138321) [details]
updated patch
------- Comment #27 From 2012-04-24 08:53:38 PST -------
(From update of attachment 138321 [details])
Nice!
------- Comment #28 From 2012-04-24 10:48:14 PST -------
(From update of attachment 138321 [details])
Clearing flags on attachment: 138321

Committed r115082: <http://trac.webkit.org/changeset/115082>
------- Comment #29 From 2012-04-24 10:48:32 PST -------
All reviewed patches have been landed.  Closing bug.