1401876 - [DateTimePicker] Picker does not close when popup is hidden

Status: VERIFIED FIXED

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

Description

[Scott Wu [:scottwu]]

The changes introduced in Bug 1390794 caused the picker not closing even when hidden.

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1401876 - (Part 1) Close datetime picker on popuphidden.

Review commit: https://reviewboard.mozilla.org/r/182082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182082/

Comment 2

[Jessica Jong [:jessica] (inactive)]

Hi Scott, I guess we just need to clean up the resources but don't need to close the picker manually, right?

Comment 3

[Scott Wu [:scottwu]]

Attachment: Bug 1401876 - (Part 2) Add browser-chrome test for reopening picker.

Review commit: https://reviewboard.mozilla.org/r/183134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183134/

Comment 4

[Scott Wu [:scottwu]]

Comment on attachment 8910639 [details]
Bug 1401876 - (Part 1) Close datetime picker on popuphidden.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182082/diff/1-2/

Comment 5

[Scott Wu [:scottwu]]

Thanks Jessica, you are right that hidePopup is redundant in closePicker, but it's still necessary in other places. I've made the change and also added a test to check for picker reopening states.

Comment 6

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

Comment on attachment 8910639 [details]
Bug 1401876 - (Part 1) Close datetime picker on popuphidden.

https://reviewboard.mozilla.org/r/182082/#review189370

Comment 7

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

Comment on attachment 8911726 [details]
Bug 1401876 - (Part 2) Add browser-chrome test for reopening picker.

https://reviewboard.mozilla.org/r/183134/#review189372

Thanks! And thanks for the test!

Comment 8

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

Comment on attachment 8911726 [details]
Bug 1401876 - (Part 2) Add browser-chrome test for reopening picker.

https://reviewboard.mozilla.org/r/183134/#review189374

Comment 9

[Scott Wu [:scottwu]]

Thanks! Checking it in.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e238860e4943
(Part 1) Close datetime picker on popuphidden. r=mconley
https://hg.mozilla.org/integration/autoland/rev/97ea12f29815
(Part 2) Add browser-chrome test for reopening picker. r=mconley

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e238860e4943
https://hg.mozilla.org/mozilla-central/rev/97ea12f29815

Comment 12

[Scott Wu [:scottwu]]

Could you also verify this bug for us Emil? It's a patch we'd like to uplift as well.

STR:
1. Open URL `data:text/html,<input type="date"/>`
2. Click the month-year button on the top
3. Click outside of the picker to close it
4. Open the picker again by clicking on the input box

Expected:
The picker should open to a clean state with calendar in view.

Actual:
The picker retains the previous state, showing month and year spinners.

Thanks!

Comment 13

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 57.0a1 (BuildId:20170920220431) by following the steps mentioned in comment 12.

This issue is verified fixed on Firefox 58.0a1 (BuildId:20171004220309) using Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.

Leaving my needinfo here as well.

Thank you Scott for the steps!

Comment 14

[Scott Wu [:scottwu]]

Comment on attachment 8910639 [details]
Bug 1401876 - (Part 1) Close datetime picker on popuphidden.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1390794 changed how popup is closed and prevented state from cleaning.
[User impact if declined]:
User could see the UI state from the previous date picker.
[Is this code covered by automated tests?]:
Yes.
[Has the fix been verified in Nightly?]:
Yes.
[Needs manual test from QE? If yes, steps to reproduce]:
Yes. STR as listed on comment 12.
[List of other uplifts needed for the feature/fix]:
Part 2 of this patch: https://bugzilla.mozilla.org/attachment.cgi?id=8911726
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
Only changes where popup closing is called.
[String changes made/needed]:
No.

Comment 15

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

Comment on attachment 8910639 [details]
Bug 1401876 - (Part 1) Close datetime picker on popuphidden.

Fixing a recent regression in DateTime picker, Beta57+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/83232ecf718c
https://hg.mozilla.org/releases/mozilla-beta/rev/65d71d7a20bd

Comment 17

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 57.0b7 (BuildId:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.