Making pinch and preventing default behavior of first touchstart event sometimes produce wrong sequence of touch events on Firefox for metro.

RESOLVED WONTFIX

Status

defect
--
minor
RESOLVED WONTFIX
6 years ago
3 months ago

People

(Reporter: nl, Assigned: nl)

Tracking

28 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [triage])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.76 Safari/537.36

Steps to reproduce:

In this bug I would like to note a potential problem that happen currently in the metroinput.cpp code.

In the metroinput.cpp we’re having the following event handling model: 
  * we catch native pointer events from OS and create widget events from them [1]
  * after that we schedule dispatching of such events [2]

In both methods we rely on the mCancelable variable that may depend on the dispatch delay. E.g. we have pinch and therefore two calls of OnPointerPressed, in the first call we have mCancelable set to true but in the second call we have mCancelable dropped to the false [3].

If we have [2] called after two calls of [1] we get mCancelable equal to false but if we have [2] called in between of two calls of [1] we get mCancelable equal to true. This race leads to different handling of pinch (or maybe other multi touch inputs). Currently it doesn't seem to break something but may in future.

[1] MetroInput::OnPointerPressed, http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#532

[2] MetroInput::DeliverNextQueuedTouchEvent, http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#1316

[3] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#577
Assignee

Updated

6 years ago
Blocks: 956644
Whiteboard: [triage]
Assignee

Updated

6 years ago
Severity: normal → minor

Updated

6 years ago
Component: Input → Widget: WinRT
Product: Firefox for Metro → Core
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [triage] → [defect] p=0

Updated

5 years ago
No longer blocks: metrobacklog, 956644
Depends on: 956644
Whiteboard: [defect] p=0
jmathies, I'm checking through the UNCONFIRMED bugs reported for Firefox 28, and I am not set up to confirm or otherwise triage this bug. Does the metro team have any particular tags you're using to track bugs like this? I will also ask around in QA to see who should be cc-ed on the bug. Thanks!
Flags: needinfo?(jmathies)
Whiteboard: metro
(In reply to Liz Henry :lizzard from comment #1)
> jmathies, I'm checking through the UNCONFIRMED bugs reported for Firefox 28,
> and I am not set up to confirm or otherwise triage this bug. Does the metro
> team have any particular tags you're using to track bugs like this? I will
> also ask around in QA to see who should be cc-ed on the bug. Thanks!

If it's something new we would add it to a triage list which we go over every week. If we want to get it fixed in a particular train we would tag it accordingly. We add [metro] to platform bugs in places like layout and gfx to keep track of stuff outside our product. (There's no need for the metro flag in widget/winrt since everything in here is metro related.)

triage tag: [triage]
train target: r=ff30
general tracking in platform: [metro] 

I'll mark this for triage again now that we've decided not to fix bug 956644.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jmathies)
Whiteboard: metro → [triage]
Thanks very much, Jim!
Assignee

Comment 4

5 years ago
Jim, I also would like to add a test for my changes. That would be an html test that imitates a pinch and checks that corrected sequence of touch events is received.
Could you let me know what would be the best place for such test? - dom/events/test or some metro specific folder?
Attachment #8380497 - Flags: review?(jmathies)
Assignee

Updated

5 years ago
Summary: Race condition over mCancelable variable in the MetroWidget code → Making pinch and preventing default behavior of first touchstart event sometimes produce wrong sequence of touch events on Firefox for metro.
Assignee

Comment 5

5 years ago
Found the impact of this issue:
Sometimes when i perform pinch and call e.preventDefault on touchstart event firefox produces start/move/cancel events but should start/move/end.
Due to the found defect i'm renaming the bug.
Comment on attachment 8380497 [details] [diff] [review]
Moved state variables initialization to the delayed method DeliverNextQueuedTouchEvent.

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

::: widget/windows/winrt/MetroInput.cpp
@@ +1340,5 @@
> +    mApzConsumingTouch = false;
> +    mCancelable = true;
> +    mCanceledIds.Clear();
> +  } else {
> +    mCancelable = false;

Should we reset this on the first touch event after the first touch start? This doesn't seem right.
(In reply to Nick Lebedev [:nl] from comment #4)
> Created attachment 8380497 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent.
> 
> Jim, I also would like to add a test for my changes. That would be an html
> test that imitates a pinch and checks that corrected sequence of touch
> events is received.
> Could you let me know what would be the best place for such test? -
> dom/events/test or some metro specific folder?

I think the only place you can do this today would be in metro mochitest - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/

Updated

5 years ago
Attachment #8380497 - Flags: review?(jmathies) → review+
Assignee

Comment 8

5 years ago
Carrying over the r+ from jimm.

To Jim: 
1. How do you think - should we move mRecognizerWantsEvents variable together with other state variables?

2. I've update patch according to your comment but got stuck with tests. To properly test this patch it would be great to simulate pinch and to check sequence of generated touch events. Seems like metro tests don't allow to check what touch events are dispatched (correct me if i'm wrong). Maybe i should proceed with dom/events tests but i'm not sure whether we can simulate a pinch in such tests. Or I maybe there is some other way to create test coverage for such patch. Could you let me know what next steps you would prefer?
Assignee: nobody → nicklebedev37
Attachment #8380497 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8385486 - Flags: review+
Flags: needinfo?(jmathies)
(In reply to Nick Lebedev [:nl] from comment #8)
> Created attachment 8385486 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent. v2.
> 
> Carrying over the r+ from jimm.
> 
> To Jim: 
> 1. How do you think - should we move mRecognizerWantsEvents variable
> together with other state variables?
> 
> 2. I've update patch according to your comment but got stuck with tests. To
> properly test this patch it would be great to simulate pinch and to check
> sequence of generated touch events. Seems like metro tests don't allow to
> check what touch events are dispatched (correct me if i'm wrong). Maybe i
> should proceed with dom/events tests but i'm not sure whether we can
> simulate a pinch in such tests. Or I maybe there is some other way to create
> test coverage for such patch. Could you let me know what next steps you
> would prefer?

Is it ok if the html page that receives the events consumes them? If so the mohcitest can inject the input, and the html page loaded in a tab can send an event to the mochitest telling it what it received. The mochitest can wait for the expected result from the page. I can point you to a test that does this, but I'm curious if you're trying to test event flow or something the apzc supposed to do..
Flags: needinfo?(jmathies)
(In reply to Nick Lebedev [:nl] from comment #8)
> Created attachment 8385486 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent. v2.
> 
> Carrying over the r+ from jimm.
> 
> To Jim: 
> 1. How do you think - should we move mRecognizerWantsEvents variable
> together with other state variables?

if it's working correctly lets leave it alone. This code is going through a simplification process anyway with the input thread separation work.
Assignee

Comment 11

5 years ago
Resolving since the code affected by my patch has already passed through huge simplification process.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX

Updated

3 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.