Closed Bug 1341691 Opened 3 years ago Closed 3 years ago

Tapping twice on the <select> combobox doesn't close it on touch devices

Categories

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

52 Branch
All
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: petruta.rasa, Assigned: kats)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

[Note]:
- Reproducible on Firefox 51 with dom.w3c_touch_events.enabled set to 2

[Affected versions]:
- Firefox 52 beta 8
- latest Aurora 53.0a2 2017-02-22
- latest Nightly 54.0a1 2017-02-21

[Affected platforms]:
- Microsoft Surface Pro with Win 10 Pro Insider build

[Steps to reproduce]:
1. Open https://bug1185394.bmoattachments.org/attachment.cgi?id=8636084
2. Tap on select dropdown - panel opens
3. Tap again on <select> combobox

[Expected result]:
- <select> panel closes

[Actual result]:
- <select> panel quickly closes and reopens 

[Regression range]:
- Regressed by bug 1298908
Kats, do you have cycles to look into this?
Flags: needinfo?(bugmail)
Yeah I'll find some time.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
Priority: -- → P1
Whiteboard: [gfx-noted]
Version: Trunk → 52 Branch
I figured out why the problem is happening. It comes from the change at [1]. When the combobox is "open", tapping on the combobox again first dismisses the popup because of the change I made, and then the tap is converted to a click which lands on the combobox and reopens it.

I'm not really sure what a good fix here is. I think we want to keep the behaviour where touching the screen outside of a popup window dismisses the popup. I think we even want that touch to allow starting a gesture (e.g. panning the page with your finger while the popup is open should close the popup and immediately let you pan). What about tapping? If you had a combobox that's open, and a button next to it, and you tapped on the button, would you expect the combobox to close and the button to get clicked? Or just one of the two?

I'll have to experiment more and see what happens on older non-touch-enabled builds and what the best thing to do here is.

[1] https://hg.mozilla.org/mozilla-central/rev/fd96f3fee975#l5.62
It looks like on older builds, or Nightly with touch events disabled, the page behaves as a user would expect - that is, tapping on the combobox while it is open will close it. Tapping on the alert button while the combobox is open will dismiss it *and* tap the button.

Programatically speaking though, the two behaviours are inconsistent because in one case a click is delivered in addition to the popup being dismissed, and in the other case the click isn't delivered. I'm not yet sure how that's happening. Will dig around more.
Ah, it has to do the consumeRollup stuff at the bottom of the function. I should have a patch soon.
Comment on attachment 8840851 [details]
Bug 1341691 - Improve handling of touch events when dismissing popups.

https://reviewboard.mozilla.org/r/115280/#review117422
Attachment #8840851 - Flags: review?(jmathies) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/174aaab139dc
Improve handling of touch events when dismissing popups. r=jimm
https://hg.mozilla.org/mozilla-central/rev/174aaab139dc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Petruta, can you please verify the issue no longer exists? Thanks!
Flags: needinfo?(petruta.rasa)
Probably too late for Fx52 at this point, but worth considering for uplift to 53 still?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Probably too late for Fx52 at this point, but worth considering for uplift
> to 53 still?

Well, in bug 1244402, touch event support was re-enabled on windows(pref on dom.w3c_touch_events.enabled:2) and ride 52 train with release note[1].
Looking into the patch, the fix seems relatively safe since it just dealt with another event WM_TOUCH.
To ensure the touch event can function normally on windows, probably we need to get it fixed and uplifted to beta 52.

[1] FF52 release note: https://developer.mozilla.org/en-US/Firefox/Releases/52
Flags: needinfo?(bugmail)
Fx52 RC1 was created on Monday. Are you saying this needs to block the release?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Fx52 RC1 was created on Monday. Are you saying this needs to block the
> release?

I don't think the symptom worth blocking the RC1 release, however, if there is a chance, e.g.RC2, probably worth doing it.
Add dev-doc-needed keyword in case we need to update MDN/release note upon change.
Keywords: dev-doc-needed
Kats, both testcases from this bug and the ones from http://webdev.cse.msu.edu/~beachjar/capstone/test.html are properly working on 54.0a1 2017-03-01 build (I would expect the fix to be available only in 2017-03-02 build).

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
Flags: needinfo?(petruta.rasa)
(In reply to Petruta Rasa [QA] [:petruta] from comment #16)
> Kats, both testcases from this bug and the ones from
> http://webdev.cse.msu.edu/~beachjar/capstone/test.html are properly working
> on 54.0a1 2017-03-01 build (I would expect the fix to be available only in
> 2017-03-02 build).

Hm, actually it looks like the fix is in the 2017-03-01 build. That was built from m-c cset 34c6c2f302e7 [1] which includes this patch.

> 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

Oh, I never tested those. I'll investigate. Thanks!

[1] http://archive.mozilla.org/pub/firefox/nightly/2017/03/2017-03-01-03-02-02-mozilla-central/firefox-54.0a1.en-US.win32.txt
(In reply to Astley Chen [:astley] (UTC+8) from comment #15)
> I don't think the symptom worth blocking the RC1 release, however, if there
> is a chance, e.g.RC2, probably worth doing it.

I don't think we should be trying to ram in changes to RC2. If it had been earlier in the beta cycle I would have requested beta uplift but given it's so late in the cycle I don't think it's a good idea.

> Add dev-doc-needed keyword in case we need to update MDN/release note upon
> change.

I don't see what developer documentation would need to change here. This is a bug, plain and simple.
Flags: needinfo?(bugmail)
Thanks for your feedback, kats. Per comment 18, update flags.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> > 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
> 
> Oh, I never tested those. I'll investigate. Thanks!

I can reproduce this and it seems to be a different root cause. I'll file a follow-up bug for this.
Thanks, marking this issue as verified in this case.
Status: RESOLVED → VERIFIED
Comment on attachment 8840851 [details]
Bug 1341691 - Improve handling of touch events when dismissing popups.

Approval Request Comment
[Feature/Bug causing the regression]: touch events enabling on windows
[User impact if declined]: when a select combobox popup is displayed, tapping on the dropdown button will close and reopen the popup, instead of just closing it.
[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]: QE verified already. STR in comment 0
[List of other uplifts needed for the feature/fix]: this can be uplifted by itself. however, there are still cases that are not completely fixed, and I'm using bug 1343977 to track those. I used a blocking relationship between the bugs but we don't need to wait for that to be fixed before uplifting this, as this provides a partial improvement already.
[Is the change risky?]: slightly
[Why is the change risky/not risky?]: the popup dismissing code is complex and I don't understand it very well. However the change seemed fairly straightforward and the difference in behaviour I introduced is restricted to touch event handling.
[String changes made/needed]: none
Attachment #8840851 - Flags: approval-mozilla-aurora?
Also note that comment 22 is requesting uplift to 53. Next week that will be beta, so consider this as a approval-mozilla-beta? if it doesn't get uplifted before the merge.
Comment on attachment 8840851 [details]
Bug 1341691 - Improve handling of touch events when dismissing popups.

Improving touch events sounds good. Let's try it for 53 aurora.
Attachment #8840851 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.