934750 - Defect - Regression: Cross-slide gesture on Start page not working

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P2)

Description

[Sam Foster [:sfoster] (he/him)]

STR: 
* Load Start page (about:start)
* Make a short swipe up or down on any tile, starting from somewhere in the middle of it

Expected results: 
* Tile should follow your swipe and snap back with an orange "selected" border and check mark

Actual results: Not much happens, it just depresses slightly (the effect used for a mouse click or tap)

Comment 1

[Matt Brubeck (:mbrubeck)]

The tile is receiving  "touchcancel" right after the first "touchmove", every time a cross-slide is attempted.  Probably a regression from bug 931504.

Comment 2

[Matt Brubeck (:mbrubeck)]

I think this was actually caused by bug 931763.  Our cross-slide implementation (bug 915328) relied on continuing to deliver events to content until APZC decided to consume them for panning or zooming.  After bug 931763, we no longer check the return status from APZC; instead we dispatch touchcancel unconditionally.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

I haven't tested this yet; waiting for a build to finish.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Don't send touchcancel to content unless APZC starts consuming input.

This includes a one-line change to APZC itself, which I don't think affects any other platforms, since it looks like only Windows widget code pays attention to the return value from ReceiveInputEvent.

Sorry this undoes a bit of the simplification from bug 931763.  We can re-simplify a lot of this eventually by using touch-action for cross slide instead of APZC hacks.

Comment 6

[Matt Brubeck (:mbrubeck)]

Work estimate p=2.  (Or should we treat this as part of the overall APZC story?)

Comment 7

[Marco Mucci [:MarcoM]]

Thanks Matt, I'll add it to the current iteration.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 827814 [details] [diff] [review]
patch

Review of attachment 827814 [details] [diff] [review]:
-----------------------------------------------------------------

I see the bug here, I'm basically filtering out content unless it calls preventDefault, which obviously isn't always the case. One question though -

::: widget/windows/winrt/MetroInput.cpp
@@ +1200,5 @@
>  
> +  // Send the event to content unless APZC is consuming it.
> +  if (!mApzConsumingTouch) {
> +    if (apzStatus == nsEventStatus_eConsumeNoDefault) {
> +      mApzConsumingTouch = true;

Why update this here? The apzc should have told us it wanted to consume on the first touchstart/touchcancel above. Beyond that, we should ignore its return results and just forward to content if !mApzConsumingTouch.

Comment 9

[Jim Mathies [:jimm]]

> Why update this here? The apzc should have told us it wanted to consume on
> the first touchstart/touchcancel above. Beyond that, we should ignore its
> return results and just forward to content if !mApzConsumingTouch.

the first touchstart/*touchmove* above

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #8)
> Why update this here? The apzc should have told us it wanted to consume on
> the first touchstart/touchcancel above. Beyond that, we should ignore its
> return results and just forward to content if !mApzConsumingTouch.

This is basically all a special case for the "cross slide" feature.  We need to wait until the pan threshold has been crossed to decide whether to start cross-sliding in content or panning in APZC.  So we need to send input to both content *and* APZC at least until the pan threshold is reached.

If we didn't need this cross-slide hack, then your original patch would be fine and we could require pages to call preventDefault if they want to receive touch events.

Comment 11

[Jim Mathies [:jimm]]

Ok, another question - if we return nsEventStatus_eIgnore while the apz is in the WAITING_LISTENERS state, why check the return result inside the |if (mCancelable) {}| block in MetroInput? The apz should be in the WAITING_LISTENERS state until we call ApzContentConsumingTouch or ApzContentIgnoringTouch (which call ContentReceivedTouch).

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #11)
> Ok, another question - if we return nsEventStatus_eIgnore while the apz is
> in the WAITING_LISTENERS state, why check the return result inside the |if
> (mCancelable) {}| block in MetroInput? The apz should be in the
> WAITING_LISTENERS state until we call ApzContentConsumingTouch or
> ApzContentIgnoringTouch (which call ContentReceivedTouch).

That's a good point. I think I did that because I didn't want to assume too much about APZC behavior, but it seems reasonable to ignore the APZC return value until after ContentReceivedTouch.  Then we could drop the part of this patch that touches AsyncPanZoomController, since it wouldn't matter.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 827814 [details] [diff] [review]
patch

Review of attachment 827814 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, sounds good. Landing this depends on kats opinion on that return result change.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 827814 [details] [diff] [review]
patch

Review of attachment 827814 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the APZC change; I didn't look at the MetroInput bits.

Comment 15

[Matt Brubeck (:mbrubeck)]

I removed the unnecessary check that jimm noted:
https://hg.mozilla.org/integration/fx-team/rev/6cf0e7cdc9a7

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6cf0e7cdc9a7

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through the following defect for iteration #20 testing without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-17-03-02-03-mozilla-central/

- Selected/De-selected several tiles from Top Sites, Bookmarks and Recent History by swiping up/down while in full view
- Selected/De-selected several tiles from Top Sites, Bookmarks and Recent History by swiping left/right while in filled view
- Ensured that once the tile has been selected, an orange border with a check mark surrounds the appropriate tile
- Ensured that once the tile has been de-selected, the orange border with the check mark is dismissed
- Ensured that the animations when swiping the tiles are smooth and responsive
- Ensured that you can select multiple tiles in the same category
- Ensured that a user can only select multiple tiles from a single category (clears all the appropriate tiles if another category is being selected)
- Ensured that all the selected tiles are cleared when pressing "ESC"
- Attempted selecting the tiles from different locations (near the top, in the middle, near the bottom of the tile)

Matt, can you take a quick look at the issues below and let me know if they are valid? I suspect #1 isn't valid and #2 is a valid issue.

Possible issues:

#1 - While in filled view, you can only select tiles by swiping left/right, doesn't work via up/down (I suspect this is intentional but not 100% sure)
#2 - I can't select any of the tiles while in snapped view (they have animations when you swipe but nothing is selected). In Windows 8.1, if Firefox Metro is using about 20% of the screen or less, you won't be able to select the tiles. Doesn't necessarily have to be snapped view as in Windows 8, the window could be a bit bigger in Windows 8.1 but the user won't be able to select the tiles.

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #17)
> #1 - While in filled view, you can only select tiles by swiping left/right,
> doesn't work via up/down (I suspect this is intentional but not 100% sure)

Yes, this is intentional.  The cross-slide gesture is always "across" from the scrollable direction, so in portrait/split/snapped view which scrolls vertically, cross-slide is horizontal.

> #2 - I can't select any of the tiles while in snapped view (they have
> animations when you swipe but nothing is selected).

I think this may be intentional, since we don't really have room to show the context appbar in snapped view -- but if this is the case, we should disable the cross-slide animation too.  Could you file a bug for this issue, please?

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Thanks Matt! Created Bug 951635 for removing the animations while Firefox Metro is in snapped view.