Closed
Bug 953012
Opened 11 years ago
Closed 7 years ago
Double Tab III: Son of Double Tab (Opening a new tab using the overlay while a website is loading creates two about:start tabs)
Categories
(Core Graveyard :: Widget: WinRT, defect, P1)
Tracking
(firefox27 unaffected, firefox28- affected, firefox29- affected, firefox30- affected)
RESOLVED
WONTFIX
People
(Reporter: kjozwiak, Unassigned)
References
Details
(Whiteboard: p=5 s=it-30c-29a-28b.3 r=ff29)
Attachments
(2 files, 4 obsolete files)
4.57 MB,
application/x-shockwave-flash
|
Details | |
17.78 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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/
Comment 1•11 years ago
|
||
Maybe related to bug 942189 and the fact that we don't have an up-to-date APZC tree while the page is loading.
Depends on: 942189
Reporter | ||
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [triage] [defect] p=0 → [beta28] [defect] p=0
Updated•11 years ago
|
Assignee: nobody → rsilveira
Updated•11 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=1
Comment 3•11 years ago
|
||
marking wfm now that the parent bug is fixed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
Comment 4•11 years ago
|
||
For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.
Reporter | ||
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
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/
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Whiteboard: [beta28] [defect] p=1 → [triage] [defect] p=0
Updated•11 years ago
|
Assignee: rsilveira → nobody
QA Contact: jbecerra
Updated•11 years ago
|
Whiteboard: [triage] [defect] p=0 → [release28] p=0
Updated•11 years ago
|
Whiteboard: [release28] p=0 → [release28] p=0 r=ff28
Comment 7•11 years ago
|
||
This happened before in bug 936005 and bug 943451 because of issues with preventDefault for chrome touch events. Bug 959792 could also be relevant.
Updated•11 years ago
|
Assignee: nobody → rsilveira
Comment 8•11 years ago
|
||
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
Attachment #8382573 -
Flags: feedback?(mbrubeck)
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Comment 9•11 years ago
|
||
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.)
Attachment #8382573 -
Flags: feedback?(mbrubeck) → feedback-
Updated•11 years ago
|
Priority: P2 → P1
QA Contact: kamiljoz
Whiteboard: [release28] p=0 r=ff28 → [release28] p=0 s=it-30c-29a-28b.3 r=ff28
Updated•11 years ago
|
Whiteboard: [release28] p=0 s=it-30c-29a-28b.3 r=ff28 → [release28] p=5 s=it-30c-29a-28b.3 r=ff28
Comment 10•11 years ago
|
||
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.
Attachment #8382573 -
Attachment is obsolete: true
Comment 12•11 years ago
|
||
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.
Attachment #8386237 -
Attachment is obsolete: true
Attachment #8386510 -
Flags: review?(mbrubeck)
Comment 13•11 years ago
|
||
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.
Updated•11 years ago
|
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
tracking-firefox28:
--- → +
tracking-firefox29:
--- → +
tracking-firefox30:
--- → +
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
Hey Rodrigo, can I remove Bug 953012 as a release 28 blocker?
Comment 16•11 years ago
|
||
(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.
Comment 17•11 years ago
|
||
Removing blocking tag and tagging for a FF29 release based on Comment #14 and #16.
Whiteboard: [release28] p=5 s=it-30c-29a-28b.3 r=ff28 → p=5 s=it-30c-29a-28b.3 r=ff29
Comment 18•11 years ago
|
||
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.
Attachment #8386510 -
Flags: review?(mbrubeck)
Attachment #8386510 -
Flags: review?(jmathies)
Attachment #8386510 -
Flags: feedback+
Updated•11 years ago
|
Component: App Bar → Widget: WinRT
Product: Firefox for Metro → Core
Comment 19•11 years ago
|
||
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.
Attachment #8386510 -
Flags: review?(jmathies) → review-
Comment 20•11 years ago
|
||
(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.
Attachment #8386510 -
Attachment is obsolete: true
Attachment #8387978 -
Flags: review?(jmathies)
Updated•11 years ago
|
Attachment #8387978 -
Flags: review?(jmathies) → review+
Comment 21•11 years ago
|
||
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!
Comment 23•11 years ago
|
||
This does not need to be tracked for fx29 anymore since metro project was cancelled.
Updated•11 years ago
|
Assignee: rsilveira → nobody
Comment 25•7 years ago
|
||
We never shipped the metro support, closing!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 7 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•