When you tap on the "+" overlay while a website is being loaded, two different about:start tabs will be created rather than just a single tab. I reproduced this several times and attached a short video. - Attached a video to illustrate the issue. Notice that when I use the overlay when polygon/facebook is loading, two about:start tabs are being created at the same time Steps to reproduce the issue: 1) Open a fresh instance of Firefox Metro (I cleared everything using the "Options" flyout) 2) Type in "polygon.com" and press "Enter". Once the website starts loading, tap on the overlay "+" button and you'll notice two different about:start tabs will be created 3) Close all the tabs and go back to a single about:start screen 4) Type in "facebook.com" press "Enter". Once the website starts loading, tap on the "+" overlay button and you'll notice two different about:start tabs will be created Current Behaviour: - When taping on the "+" overlay while a website is being loaded, two different about:start tabs are being created rather than one Expected Behaviour: - One a single about:start tab should be created, not two different tabs. Found the issue using the following build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-23-03-02-03-mozilla-central/

Maybe related to bug 942189 and the fact that we don't have an up-to-date APZC tree while the page is loading.

I was playing around with Facebook for about 10 minutes before opening a new tab using the overlay and two about:start tabs were created. Not sure if this is directly linked to loading but it does happen reliably during that loading phase.

marking wfm now that the parent bug is fixed.

For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.

This is definitely still happening but is really hard to reproduce. Sometimes when creating new tabs, two tabs will open rather than just the one. I will keep my eye on this when going through the next iteration and see if I can get some reliable STR.

Moving this back into the backlog as I can reproduce the issue very consistently with both the X1 Carbon and the surface pro 2. I've attached a video to illustrate the issue: - http://youtu.be/NgioUM6CUOA Reproduced using the following builds: - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-11-03-02-01-mozilla-central/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-11-00-40-37-mozilla-aurora/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/

This happened before in bug 936005 and bug 943451 because of issues with preventDefault for chrome touch events. Bug 959792 could also be relevant.

Quoting mbrubeck on #windev[1]: "maybe in perf-constrained situations, OnPointerReleased gets called before DeliverNextQueuedTouchEvent has had a chance to process the touchstart event." "if we take too long to dequeue the pointerstart, then this code will not yet see that gesture recognition has been canceled [2]" This fix moves the call mGestureRecognizer->ProcessUpEvent() inside DeliverNextQueuedTouchEvent() while processing a touch start or touch end event. [1] - http://logbot.glob.com.au/?c=mozilla%23windev&s=25+Feb+2014&e=25+Feb+2014#c12984 [2] - http://dxr.mozilla.org/mozilla-ce...indows/winrt/MetroInput.cpp#712

Comment on attachment 8382573 [details] [diff] [review] 953012.patch Review of attachment 8382573 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/winrt/MetroInput.cpp @@ +708,5 @@ > new WidgetTouchEvent(true, NS_TOUCH_END, mWidget.Get()); > touchEvent->touches.AppendElement(CreateDOMTouch(currentPoint.Get())); > DispatchAsyncTouchEvent(touchEvent); > > + mTouchEndPoint = currentPoint; I think this could go wrong if OnPointerReleased is called again (for a second pointer) before the the first touch event has been dispatched. To be guaranteed correct, we really need to add currentPoint to a queue (just like touchEvent). Also, we should probably do the same for the other mGestureRecognizer->Process* calls, to avoid changing the order of events seen by the gesture recognizer. Perhaps we could add a second argument to DispatchAsyncTouchEvent, and it could put both the WidgetTouchEvent and the IPointerPoint into a queue (or two parallel queues). Or maybe there's a simpler way to do this that I haven't thought of... (In JavaScript I would have DispatchAsyncTouchEvent return a promise, and just add a "yield" here.)

Adding the gesture recognizer arguments to the input event queue and processing them when the touch event is delivered. I'll do some cleanup and handle memory allocation properly in InputEventQueueEntry.

Forgot to qref.

I tried simplifying the usage of TouchEventQueueEntry by storing IPointerEventArgs instead of having both IPointerPoint and PointerPointVector but the IPointerEventArgs passed to OnPointer* gets de-alocated after the method returns.

Yes, we are going to build with our final beta today - let's get a nomination here for uplift with a breakdown of risk/reward. Is this a metro-only fix? That will make it more likely to be ok for uplift.

I'm not comfortable uplifting this to beta without some baking time in m-c. I don't think it meets the bar for beta at this point either.

Hey Rodrigo, can I remove Bug 953012 as a release 28 blocker?

(In reply to Rodrigo Silveira [:rsilveira] from comment #14) > I'm not comfortable uplifting this to beta without some baking time in m-c. > I don't think it meets the bar for beta at this point either. Thank you for that assessment, we will not track this for FF28 in that case and it can get bake time on nightly (soon to be Aurora) with consideration for FF29 if safe enough.

Removing blocking tag and tagging for a FF29 release based on Comment #14 and #16.

Comment on attachment 8386510 [details] [diff] [review] Patch v1 Review of attachment 8386510 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but I want Jim to do the final review on this.

Comment on attachment 8386510 [details] [diff] [review] Patch v1 Review of attachment 8386510 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks great, although there's one issue to fix. ::: widget/windows/winrt/MetroInput.cpp @@ +656,5 @@ > new WidgetTouchEvent(true, NS_TOUCH_MOVE, mWidget.Get()); > InitTouchEventTouchList(touchEvent); > + > + Foundation::Collections::IVector<UI::Input::PointerPoint*>* pointerPoints; > + aArgs->GetIntermediatePoints(&pointerPoints); The recognizer needs the intermediate points even if the point hasn't moved. This is why AddPointerMoveDataToRecognizer(aArgs) was called before the HasPointMoved check. I've confirmed this work regresses bug 918937. We'll have to find a way around that. Now that we have native touch injection apis we could write a long-tap test. hint hint, nudge nudge. :P @@ -1357,3 @@ > MOZ_ASSERT(event); > > - AutoDeleteEvent wrap(event); Please remove the implementation of AutoDeleteEvent as well.

(In reply to Jim Mathies [:jimm] from comment #19) > Comment on attachment 8386510 [details] [diff] [review] > Patch v1 > ::: widget/windows/winrt/MetroInput.cpp > @@ +656,5 @@ > > new WidgetTouchEvent(true, NS_TOUCH_MOVE, mWidget.Get()); > > InitTouchEventTouchList(touchEvent); > > + > > + Foundation::Collections::IVector<UI::Input::PointerPoint*>* pointerPoints; > > + aArgs->GetIntermediatePoints(&pointerPoints); > > The recognizer needs the intermediate points even if the point hasn't moved. > This is why AddPointerMoveDataToRecognizer(aArgs) was called before the > HasPointMoved check. I've confirmed this work regresses bug 918937. We'll > have to find a way around that. > Thanks for catching this! I'm now sending !HasPointMoved the touch move event to the gesture recognizer asynchronously, to make sure the event order is kept > Now that we have native touch injection apis we could write a long-tap test. > hint hint, nudge nudge. :P > Couldn't find a bug, so created bug 981212 to track this. > @@ -1357,3 @@ > > MOZ_ASSERT(event); > > > > - AutoDeleteEvent wrap(event); > > Please remove the implementation of AutoDeleteEvent as well. Done.

Why not? https://hg.mozilla.org/integration/fx-team/rev/c6fc27e68f45

Backed out in https://hg.mozilla.org/integration/fx-team/rev/48680af02f8f for introducing a m-mc test failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35960417&tree=Fx-Team Why not, indeed!

This does not need to be tracked for fx29 anymore since metro project was cancelled.

no longer blocking release as per comment 23

We never shipped the metro support, closing!