943451 - Defect - Return of double tab! (touchstart.preventDefault does not prevent clicks in Metro chrome)

Status: VERIFIED FIXED

(Core Graveyard :: Widget: WinRT, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: backout-chrome-hit

When clicking the "new tab" overlay button on the right edge of the screen in Metro Firefox, I see two new tabs appear instead of one.

This is identical bug 936005 which was previously fixed.  This time it is a regression from the "part 1" patch from bug 941324.  When we stopped sending chrome touches to APZC, we also stopped checking whether to cancel gesture recognition.

This patch backs out "part 1" from bug 941324, then adds one line to restore a fix from that patch to ensure that only the first "touchstart" event can be canceled.

Comment 1

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8338613 [details] [diff] [review]
backout-chrome-hit

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

I noticed some possible opportunities for cleanup here:

::: widget/windows/winrt/MetroInput.cpp
@@ +1211,5 @@
>      // Only translate if we're dealing with web content that's transformed
>      // by the apzc.
> +    if (!mChromeHitTestCacheForTouch) {
> +      TransformTouchEvent(event);
> +    }

It's not actually possible for mChromeHitTestCacheForTouch to be true here.  (If it's true, then we'll always bail out in either the !mCancelable or mCancelable code paths above.)  Should we leave these checks in for robustness anyways?  Or should we just MOZ_ASSERT(!mChromeHitTestCacheForTouch) when we get to this point?

@@ +1218,5 @@
>      return;
>    }
>  
>    DUMP_TOUCH_IDS("APZC(2)", event);
>    status = mWidget->ApzReceiveInputEvent(event, nullptr);

Should we pass a transformedEvent argument here, so we don't need to call TransformTouchEvent separately below?

Comment 2

[Marco Mucci [:MarcoM]]

Hi Matt, can you provide a point value for this defect.  Thanks.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Comment on attachment 8338613 [details] [diff] [review]
> backout-chrome-hit
> 
> Review of attachment 8338613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I noticed some possible opportunities for cleanup here:
> 
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +1211,5 @@
> >      // Only translate if we're dealing with web content that's transformed
> >      // by the apzc.
> > +    if (!mChromeHitTestCacheForTouch) {
> > +      TransformTouchEvent(event);
> > +    }
> 
> It's not actually possible for mChromeHitTestCacheForTouch to be true here. 
> (If it's true, then we'll always bail out in either the !mCancelable or
> mCancelable code paths above.)  Should we leave these checks in for
> robustness anyways?  Or should we just
> MOZ_ASSERT(!mChromeHitTestCacheForTouch) when we get to this point?

An assert seems fine. 
 
> @@ +1218,5 @@
> >      return;
> >    }
> >  
> >    DUMP_TOUCH_IDS("APZC(2)", event);
> >    status = mWidget->ApzReceiveInputEvent(event, nullptr);
> 
> Should we pass a transformedEvent argument here, so we don't need to call
> TransformTouchEvent separately below?

Seems fine to me either way.

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Marco Mucci [:MarcoM] from comment #2)
> Hi Matt, can you provide a point value for this defect.  Thanks.

p=1

Comment 5

[Matt Brubeck (:mbrubeck)]

Landed.  I didn't end up doing the cleanups because I wanted to keep this as close to a simple backout as possible, to avoid any more accidental regressions:

https://hg.mozilla.org/integration/fx-team/rev/6ca43b5171c1

Comment 6

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

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

Comment 7

[Matt Brubeck (:mbrubeck)]

Backed out for causing bug 944178:
https://hg.mozilla.org/mozilla-central/rev/39c59a2ed8c7

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: alternate patch

Allow chrome to cancel touchstart events, but continue bypassing APZC to avoid confusing it.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/3fc2445c1726

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fc2445c1726

Comment 11

[Manuela Muntean [Away]]

Verified as fixed with Firefox 28 beta 7 on a Dell XPS 12 ultrabook with Windows 8 64-bit: when clicking the "new tab" overlay button on the right edge of the screen in Metro Firefox, I see only one tab appear.