Bug 84350 - [Qt][WK2] Implement axis locking on the WebView for pan gestures
Summary: [Qt][WK2] Implement axis locking on the WebView for pan gestures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P3 Normal
Assignee: Andras Becsi
URL:
Keywords: Qt
Depends on:
Blocks: 76773
  Show dependency treegraph
 
Reported: 2012-04-19 09:57 PDT by Andras Becsi
Modified: 2012-04-24 10:48 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2012-04-19 09:57:02 PDT
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 Andras Becsi 2012-04-19 10:17:22 PDT
Created attachment 137916 [details]
proposed patch
Comment 2 zalan 2012-04-19 11:27:25 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2012-04-19 16:28:21 PDT
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.
Comment 4 Andras Becsi 2012-04-20 03:02:29 PDT
(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.
Comment 5 Andras Becsi 2012-04-20 03:13:14 PDT
(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.
Comment 6 Andras Becsi 2012-04-20 03:14:09 PDT
Created attachment 138065 [details]
proposed patch v2
Comment 7 zalan 2012-04-20 03:40:29 PDT
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.
Comment 8 Andras Becsi 2012-04-20 04:34:56 PDT
(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.
Comment 9 zalan 2012-04-20 04:52:08 PDT
> 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 Andras Becsi 2012-04-20 05:18:19 PDT
(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 Andras Becsi 2012-04-20 05:31:03 PDT
Created attachment 138074 [details]
proposed patch
Comment 12 Andras Becsi 2012-04-20 05:46:46 PDT
Created attachment 138075 [details]
updated patch

Add a missing hunk.
Comment 13 Kenneth Rohde Christiansen 2012-04-20 09:36:32 PDT
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?
Comment 14 Andras Becsi 2012-04-20 11:19:49 PDT
Created attachment 138124 [details]
proposed patch

Addressed comments and further tweaked the constraints to behave better on the device.
Comment 15 zalan 2012-04-20 11:35:12 PDT
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.
Comment 16 Andras Becsi 2012-04-20 11:43:22 PDT
(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.
Comment 17 zalan 2012-04-20 11:50:54 PDT
(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.
Comment 18 Andras Becsi 2012-04-20 12:15:26 PDT
Created attachment 138141 [details]
patch for review
Comment 19 Kenneth Rohde Christiansen 2012-04-21 09:48:05 PDT
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) { }
Comment 20 Andras Becsi 2012-04-21 12:10:01 PDT
(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.
Comment 21 Andras Becsi 2012-04-21 14:28:27 PDT
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 Kenneth Rohde Christiansen 2012-04-22 09:04:03 PDT
(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 zalan 2012-04-22 10:00:40 PDT
(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 Andras Becsi 2012-04-22 10:34:10 PDT
(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 zalan 2012-04-22 10:45:16 PDT
(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 Andras Becsi 2012-04-23 03:35:54 PDT
Created attachment 138321 [details]
updated patch
Comment 27 Kenneth Rohde Christiansen 2012-04-24 08:53:38 PDT
Comment on attachment 138321 [details]
updated patch

Nice!
Comment 28 Andras Becsi 2012-04-24 10:48:14 PDT
Comment on attachment 138321 [details]
updated patch

Clearing flags on attachment: 138321

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