Summary: | [Qt] Fix axis locking when panning on N9 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lauro Moura Maranhao Neto <lauro.neto> | ||||||||||
Component: | WebKit Qt | Assignee: | Lauro Moura Maranhao Neto <lauro.neto> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abecsi, cmarcelo, hausmann, kenneth, menard, webkit.review.bot, zoltan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Lauro Moura Maranhao Neto
2012-07-26 10:35:37 PDT
Created attachment 154714 [details]
Patch
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). (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. Created attachment 155079 [details]
Patch
Andras what do you think about this? Btw, remember adding punctuation mark to the end of comments 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. 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?
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. (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. (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. 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? Created attachment 156085 [details]
Patch
Updated the patch initializing m_time.
Comment on attachment 156085 [details] Patch Clearing flags on attachment: 156085 Committed r124462: <http://trac.webkit.org/changeset/124462> All reviewed patches have been landed. Closing bug. |