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

VERIFIED FIXED in mozilla28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
4 months ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

({regression})

28 Branch
mozilla28
All
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch backout-chrome-hit (obsolete) — Splinter Review
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.
Attachment #8338613 - Flags: review?(jmathies)
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?
Attachment #8338613 - Flags: review?(jmathies) → review+
Hi Matt, can you provide a point value for this defect.  Thanks.
Blocks: metrov1it20
Flags: needinfo?(mbrubeck)
Priority: -- → P2
QA Contact: jbecerra
Summary: Return of double tab! (touchstart.preventDefault does not prevent clicks in Metro chrome) → Defect - Return of double tab! (touchstart.preventDefault does not prevent clicks in Metro chrome)
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0
(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.
(In reply to Marco Mucci [:MarcoM] from comment #2)
> Hi Matt, can you provide a point value for this defect.  Thanks.

p=1
Flags: needinfo?(mbrubeck)
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=1
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
https://hg.mozilla.org/mozilla-central/rev/6ca43b5171c1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Backed out for causing bug 944178:
https://hg.mozilla.org/mozilla-central/rev/39c59a2ed8c7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Allow chrome to cancel touchstart events, but continue bypassing APZC to avoid confusing it.
Attachment #8338613 - Attachment is obsolete: true
Attachment #8339759 - Flags: review?(jmathies)
No longer blocks: 944178
Depends on: 944178
Attachment #8339759 - Flags: review?(jmathies) → review+
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/3fc2445c1726
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Keywords: verifyme
Depends on: 953012
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.