1406859 - [DateTimePicker] Picker does not show if clicking on the second date input on the same page

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Scott Wu [:scottwu]]

STR
1: Open URL `data:text/html,<input type="date"><input type="date">`.
2: Click on the first date input box.
3: Click on the second date input box.
4: Click on the first date input box.
Expected:
Date picker should pop up under the first date input box.
Actual:
Date picker does not open for the first date input box, bug works for the second input box.

Tested fine on Firefox 56, so could be related to bug 1390794.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Yeah, this should be related to bug 1390794. Before that bug, we close the picker manually when input blurs and now we rely on the picker's default behavior, which is to close when clicking outside the panel or the anchored element.

In this case, when the second date input box is clicked, we send 'MozOpenDateTimePicker' event and this._mInputElement is reset in [1] before the first picker is closed gracefully, later the 'popuphidden' event of the first picker is received and the states got messed up.

Let me find a way to fix it...

[1] http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/content/browser-content.js#1793

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Using the STR in comment 0, I could reproduce this on Windows and macOS but not on Ubuntu.
It depends on _when_ we receive the 'popuphidden' event. On Ubuntu, we receive the 'popuphidden' event before we send 'MozOpenDateTimePicker' for the second date input, so everything is fine. On cases that things go wrong is because we receive the 'popuphidden' event when we are already opening the second picker.

I tried to close the first picker manually before opening the second picker, but I always get the 'popuphidden' event of the first picker when re-adding listeners for the second picker, so the second picker gets closed immediately.
It looks like we need consume the 'popuphidden' event, otherwise it'd be fired when re-adding the listener. The drawback would be that when clicking on the second input box, the first picker will close, and we need to click again for the second picker to open.

Mike, do you think this solution is acceptable? Thanks.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8917286 [details] [diff] [review]
patch, v1.

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

Great, thanks! Would it be possible to add a test for this in toolkit/content/tests/browser/browser_datetime_datepicker.js ? Okay if you want to do that in a follow-up bug.

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #3)
> Comment on attachment 8917286 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8917286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks! Would it be possible to add a test for this in
> toolkit/content/tests/browser/browser_datetime_datepicker.js ? Okay if you
> want to do that in a follow-up bug.

I tried to add a test for it, but it looks like using BrowserTestUtils.synthesizeMouseAtCenter on the second input doesn't cause the first picker to rollup. That is because the rollup is called from the widget level [1], and I'm not sure how to (or whether we can) synthesize the event in the widget level.

I'll land this patch first to fix the regression and see how to add a test for it.

[1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/widget/nsIRollupListener.h#35

Comment 5

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ec6d63f4d5cd2565ec5979205e98884f6bde4c1&group_state=expanded

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629afbb52163
[DateTimePicker] Let the first picker close gracefully before opening a second picker. r=mconley

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/629afbb52163

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta or can it ride the 58 train?

Comment 9

[Jessica Jong [:jessica] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> Is this something we should consider uplifting to Beta or can it ride the 58
> train?

I think we should uplift this to Beta, let me request for approval.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8917286 [details] [diff] [review]
patch, v1.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1390794
[User impact if declined]: web pages with two date/time inputs might not work correctly when clicking on the second input while another picker is opened
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes, verified using 2017-10-19 on mac
[Needs manual test from QE? If yes, steps to reproduce]: yes, STR is in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minimal change
[String changes made/needed]: none

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917286 [details] [diff] [review]
patch, v1.

Seems like a must fix, Beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dfee079363e1

Comment 13

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0 using an affected Firefox 58.0a1 build (BuildId:20171009220104).

I have verified that this issue is not reproducible using Firefox 58.0a1 (BuildId:20171022220103) and Firefox 57.0b11 (Build Id:20171023180840) on Windows 10 64bit, macOS 10.9.5 and Ubuntu 12.04 32bit.