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

RESOLVED FIXED in Firefox 28

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfoster, Assigned: mbrubeck)

Tracking

({regression})

unspecified
Firefox 28
x86
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [block28] feature=defect c=tbd u=tbd p=2)

Attachments

(1 attachment, 2 obsolete attachments)

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)
Summary: Regression: Cross-slide gesture on Start page not working → Defect - Regression: Cross-slide gesture on Start page not working
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0
The tile is receiving  "touchcancel" right after the first "touchmove", every time a cross-slide is attempted.  Probably a regression from bug 931504.
Blocks: 931504
Keywords: regression
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.
Blocks: 931763
No longer blocks: 931504
Posted patch WIP (obsolete) — Splinter Review
I haven't tested this yet; waiting for a build to finish.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Posted patch WIP 2 (obsolete) — Splinter Review
Attachment #827737 - Attachment is obsolete: true
Posted patch patchSplinter Review
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.
Attachment #827739 - Attachment is obsolete: true
Attachment #827814 - Flags: review?(jmathies)
Attachment #827814 - Flags: review?(bugmail.mozilla)
Work estimate p=2.  (Or should we treat this as part of the overall APZC story?)
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → [triage] feature=defect c=tbd u=tbd p=2
Thanks Matt, I'll add it to the current iteration.
Blocks: metrov1it18
No longer blocks: metrov1backlog
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [triage] feature=defect c=tbd u=tbd p=2 → [block28] feature=defect c=tbd u=tbd p=2
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.
> 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
(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.
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).
(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 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.
Attachment #827814 - Flags: review?(jmathies) → review+
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.
Attachment #827814 - Flags: review?(bugmail.mozilla) → review+
I removed the unnecessary check that jimm noted:
https://hg.mozilla.org/integration/fx-team/rev/6cf0e7cdc9a7
https://hg.mozilla.org/mozilla-central/rev/6cf0e7cdc9a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
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.
Flags: needinfo?(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?
Flags: needinfo?(mbrubeck)
Thanks Matt! Created Bug 951635 for removing the animations while Firefox Metro is in snapped view.
You need to log in before you can comment on or make changes to this bug.