Closed Bug 1406859 Opened 5 years ago Closed 5 years ago

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

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: scottwu, Assigned: jessica)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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
Assignee: nobody → jjong
Attached patch patch, v1.Splinter Review
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.
Attachment #8917286 - Flags: review?(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.
Attachment #8917286 - Flags: review?(mconley) → review+
(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
Pushed by jjong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629afbb52163
[DateTimePicker] Let the first picker close gracefully before opening a second picker. r=mconley
https://hg.mozilla.org/mozilla-central/rev/629afbb52163
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this something we should consider uplifting to Beta or can it ride the 58 train?
Flags: needinfo?(jjong)
(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.
Flags: needinfo?(jjong)
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
Attachment #8917286 - Flags: approval-mozilla-beta?
Blocks: 1410290
Comment on attachment 8917286 [details] [diff] [review]
patch, v1.

Seems like a must fix, Beta57+
Attachment #8917286 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.