Closed
Bug 943451
Opened 11 years ago
Closed 11 years ago
Defect - Return of double tab! (touchstart.preventDefault does not prevent clicks in Metro chrome)
Categories
(Core Graveyard :: Widget: WinRT, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla28
People
(Reporter: mbrubeck, Assigned: mbrubeck)
References
Details
(Keywords: regression, Whiteboard: [block28] feature=defect c=tbd u=tbd p=1)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8338613 -
Flags: review?(jmathies) → review+
Comment 2•11 years ago
|
||
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
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
(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)
Updated•11 years ago
|
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=1
Assignee | ||
Comment 5•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assignee | ||
Comment 7•11 years ago
|
||
Backed out for causing bug 944178:
https://hg.mozilla.org/mozilla-central/rev/39c59a2ed8c7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•11 years ago
|
||
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)
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8339759 -
Flags: review?(jmathies) → review+
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 11•11 years ago
|
||
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
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
•