Closed Bug 1343977 Opened 7 years ago Closed 7 years ago

Tapping twice on chrome popups doesn't close it on touch devices

Categories

(Core :: Panning and Zooming, defect, P2)

52 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(2 files)

Spinoff from bug 1341691. That bug was about <select> comboboxes, and was fixed. However:

Petruta Rasa [QA] [:petruta] from bug 1341691 comment #16:
> The issues still occurs on:
> - Customization page (Menu panel - Customize) for "Show / Hide Toolbars"
> dropdown from the bottom of the page.  
> - Library window for "Organize", "Views" and "Import and Backup" options

The customization page doesn't actually use <select>, instead it uses a <button> with a menu popup [1], and the Library window uses a <menu> item [2].

For some reason in these cases, the `consumeRollupEvent` comes out as 0 for both touch and mouse, and yet with mouse the popup closes but with touch it reopens. This is different from what I was seeing in bug 1341691, so the problem is somewhere else.

[1] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/components/customizableui/content/customizeMode.inc.xul#29
[2] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/components/places/content/places.xul#161
Ok, so I think the problem is still somehow related to the consumption of rollup events. In particular, when I do a touch-tap on the button to close the popup, the touch event handling goes through the code at [1], so the event is not consumed. This means the touch event will dismiss the popup, but since it's not consumed, it also fires a click event, and the click event lands on the button that reopens the popup.

What I don't understand is why mouse-clicking behaves differently. I would expect the mousedown to dismiss the popup, and the mouseclick event to fire on the button and reopen the popup. Clearly that's not happening, and I've verified that a click event is in fact firing with the button as the target. I'm not sure where/how the click event is ignored so that it doesn't reopen the popup. Neil, do you know?

[1] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/layout/xul/nsMenuPopupFrame.cpp#1775
Flags: needinfo?(enndeakin)
I assume by 'mouse click' you mean a WM_LBUTTONDOWN mouse down event?

Without testing it I would guess that nsBaseWidget::mLastRollup is being assigned during the touch event and cleared at the end of the event when nsAutoRollup goes out of scope, so the code in MayShowPopup doesn't prevent the popup from reopening for the later mouse event.
Flags: needinfo?(enndeakin)
Thanks! That appears to be the problem (or at least one of the problems). The click that is triggered by touch events happens asynchronously, so the nsAutoRollup is no longer on the stack at that point.
Comment on attachment 8845063 [details]
Bug 1343977 - Ensure that synthetic mouseclicks generated from touch gestures don't reopen rollups.

https://reviewboard.mozilla.org/r/118282/#review121204

Might we get into situations like this:

  - We get a touch-start for touch A that sets APZEventState::mTouchRollup
    to one thing.
    
  - We get a touch-start for touch B that sets APZEventState::mTouchRollup
    to another thing.
    
  - We get a single-tap for touch A which looks at APZEventState::mTouchRollup
    and picks up the wrong value?
    
Also, just a heads up that your intended review requests to Neil did not get turned into Bugzilla r? flags. (Neil, this is in turn because you don't have the magic |:enndeakin| string in your Bugzilla profile name; you may want to consider adding that.)

::: gfx/layers/apz/util/APZEventState.cpp:184
(Diff revision 1)
>  {
>    APZES_LOG("Handling single tap at %s on %s with %d\n",
>      Stringify(aPoint).c_str(), Stringify(aGuid).c_str(), mTouchEndCancelled);
>  
> +  RefPtr<nsIContent> touchRollup = GetTouchRollup();
> +  mozilla::widget::nsAutoRollup rollup(touchRollup.get());

I would find it slightly clearer if this were scoped to the |if| block below that calls APZCCallbackHelper::FireSingleTapEvent, and |touchRollup| was passed to the DelayedFireSingleTapEvent constructor explicitly.

::: gfx/layers/apz/util/APZEventState.cpp:334
(Diff revision 1)
>    APZES_LOG("Handling event type %d\n", aEvent.mMessage);
>    switch (aEvent.mMessage) {
>    case eTouchStart: {
>      mTouchEndCancelled = false;
> +    nsresult rv;
> +    mTouchRollup = do_GetWeakReference(mozilla::widget::nsAutoRollup::GetLastRollup(), &rv);

The second argument to do_GetWeakReference() is optional. If you don't intend to use it, don't pass it at all.

Also, |mozilla::| is redundant here.
(In reply to Botond Ballo [:botond] from comment #7)
> Might we get into situations like this:
> 
>   - We get a touch-start for touch A that sets APZEventState::mTouchRollup
>     to one thing.
>     
>   - We get a touch-start for touch B that sets APZEventState::mTouchRollup
>     to another thing.
>     
>   - We get a single-tap for touch A which looks at
> APZEventState::mTouchRollup
>     and picks up the wrong value?

I guess it is possible, yes. I feel like we need to do a bit of overhaul on APZEventState and ActiveElementManager to make sure that touch events and "other" notifications that come in via the GeckoContentController API for a particular input block are tracked properly and we don't end up doing a mix-and-match thing. The general problem is what I believe is behind bug 1227241 as well.

Would you be ok with me filing a follow-up bug for this? Or would you rather we held back this fix until we get that sorted out? My feeling is that these patches would be an incremental improvement even without handling this edge case.

> Also, just a heads up that your intended review requests to Neil did not get
> turned into Bugzilla r? flags.

Thanks for the heads up! I hadn't noticed. Boo silent failure.

> I would find it slightly clearer if this were scoped to the |if| block below
> that calls APZCCallbackHelper::FireSingleTapEvent, and |touchRollup| was
> passed to the DelayedFireSingleTapEvent constructor explicitly.

Makes sense, I'll update that.

> The second argument to do_GetWeakReference() is optional. If you don't
> intend to use it, don't pass it at all.
> 
> Also, |mozilla::| is redundant here.

Will fix that, thanks.
Attachment #8845062 - Flags: review?(enndeakin)
Attachment #8845063 - Flags: review?(enndeakin)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Botond Ballo [:botond] from comment #7)
> > Might we get into situations like this:
> > 
> >   - We get a touch-start for touch A that sets APZEventState::mTouchRollup
> >     to one thing.
> >     
> >   - We get a touch-start for touch B that sets APZEventState::mTouchRollup
> >     to another thing.
> >     
> >   - We get a single-tap for touch A which looks at
> > APZEventState::mTouchRollup
> >     and picks up the wrong value?
> 
> I guess it is possible, yes. I feel like we need to do a bit of overhaul on
> APZEventState and ActiveElementManager to make sure that touch events and
> "other" notifications that come in via the GeckoContentController API for a
> particular input block are tracked properly and we don't end up doing a
> mix-and-match thing. The general problem is what I believe is behind bug
> 1227241 as well.
> 
> Would you be ok with me filing a follow-up bug for this? Or would you rather
> we held back this fix until we get that sorted out? My feeling is that these
> patches would be an incremental improvement even without handling this edge
> case.

Landing this as-is is fine, I just wanted to make sure we're aware of the issue. Maybe add a comment to the code, or make a note in bug 1227241 to consider mTouchRollup when we do fix it?
Comment on attachment 8845063 [details]
Bug 1343977 - Ensure that synthetic mouseclicks generated from touch gestures don't reopen rollups.

https://reviewboard.mozilla.org/r/118282/#review121246
Attachment #8845063 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #9)
> Landing this as-is is fine, I just wanted to make sure we're aware of the
> issue. Maybe add a comment to the code, or make a note in bug 1227241 to
> consider mTouchRollup when we do fix it?

Thanks. I added a comment in the code and also left a comment in bug 1227241.
Attachment #8845062 - Flags: review?(enndeakin)
Attachment #8845063 - Flags: review?(enndeakin)
Comment on attachment 8845062 [details]
Bug 1343977 - Extract nsAutoRollup into a more self-contained class and clean it up some. No functional changes intended.

https://reviewboard.mozilla.org/r/118280/#review122082
Attachment #8845062 - Flags: review?(enndeakin) → review+
Comment on attachment 8845063 [details]
Bug 1343977 - Ensure that synthetic mouseclicks generated from touch gestures don't reopen rollups.

https://reviewboard.mozilla.org/r/118282/#review122088
Attachment #8845063 - Flags: review?(enndeakin) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dcb0cd0ee36
Extract nsAutoRollup into a more self-contained class and clean it up some. No functional changes intended. r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/40479d157fc8
Ensure that synthetic mouseclicks generated from touch gestures don't reopen rollups. r=botond,enndeakin+6102
https://hg.mozilla.org/mozilla-central/rev/8dcb0cd0ee36
https://hg.mozilla.org/mozilla-central/rev/40479d157fc8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Petruta, would you please verify the fix on the next Nightly? Thanks! If it's good I will request uplift to aurora and beta.
Flags: needinfo?(petruta.rasa)
On 55.0a1 Nightly 2017-03-16 build, the options from Library window are closing on second tap and they look just like on older builds.
On the other hand, "Show/Hide Toolbars" button from Customize page has a short flicker on second tap - it becomes white, then gray and back to white. Also, more rarely, although the pop-up is closed the button remains selected (pressed).
Flags: needinfo?(petruta.rasa)
Depends on: 1348238
No longer depends on: 1348238
(In reply to Petruta Rasa [QA] [:petruta] from comment #19)
> On the other hand, "Show/Hide Toolbars" button from Customize page has a
> short flicker on second tap - it becomes white, then gray and back to white.
> Also, more rarely, although the pop-up is closed the button remains selected
> (pressed).

I filed bug 1349291 for this.
Also for the record I do still want to uplift this bug to aurora 54/beta 53. However there's at least one crasher regression (bug 1349187) from it so I'd like to give it a bit more bake time.
Comment on attachment 8845062 [details]
Bug 1343977 - Extract nsAutoRollup into a more self-contained class and clean it up some. No functional changes intended.

Approval Request Comment
[Feature/Bug causing the regression]: APZ touch support
[User impact if declined]: in some places in the browser UI, dismissing select popup dialogs by tapping on the select button immediately reopens the popup
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes (after uplift). On a touch-enabled windows device, go into the toolbar customize mode, tap on the "show/hide toolbars" button to open the popup, and then tap on it again to close it. It should stay closed instead of reopening. Test similarly on the other buttons listed in comment 0.
[List of other uplifts needed for the feature/fix]: bug 1349187
[Is the change risky?]: a little bit
[Why is the change risky/not risky?]: The change isn't massive, but it's not trivial either. One regression was already reported (and fixed) but it's possible there's more. However the code is fairly well understood.
[String changes made/needed]: none
Attachment #8845062 - Flags: approval-mozilla-beta?
Attachment #8845062 - Flags: approval-mozilla-aurora?
Hi Ioana, could you help find someone to verify if this issue and bug 1349187 were fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
Hey Gerry, yes - Petruta will look at it as she worked on the bug till now. 
Thanks!
Flags: needinfo?(ioana.chiorean)
This is verified on Nightly 55 as per comment 19, the remaining issues were logged separately. Currently I can't reproduce bug 1349187 and I asked for more details there.
Status: RESOLVED → VERIFIED
Comment on attachment 8845062 [details]
Bug 1343977 - Extract nsAutoRollup into a more self-contained class and clean it up some. No functional changes intended.

Fix a regression related to tapping popups on touch devices and was verified. Aurora54+.
Attachment #8845062 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8845062 [details]
Bug 1343977 - Extract nsAutoRollup into a more self-contained class and clean it up some. No functional changes intended.

Since there are other ways to close popups and this looks a bit complicated, I'd like to let this ride with 54. We are getting towards the end of beta 53 at this point.
Attachment #8845062 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1355053
Verified as fixed on Firefox 54 beta 1 on a Microsoft Pro 2 tablet with Win 10 64-bit.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: