1183037 - Once displayed, form autocomplete popup will not display after the second time

Status: VERIFIED FIXED

(Core :: Widget, defect, P2)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: autocomplete_sample.html

Once displayed, Autocomplete popup will not display after the second time.

Reproduce Step:
 1) enable e10s.
 2) open the sample page(attachment file)
 3) double-click the input element. (displayed autocomplete suggestion.)
 4) click the input element. (hide the auto complete popup.)
 5) click the input element.

Expected Result:
 display the autocomplete popup again.

Actual Result: 
 doesn't displayed the autocomplete popup.

Additional Information:
 - it can't reproduce this phenomenon when disable e10s. 
 - this phenomenon occurred on 41 and 42.
 - it can reproduce on the OS X environment.
 - it can display popup when re-focused the input element.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi,

I have tested this issue on latest Aurora (46.0a2) build, latest Nightly (47.0a1) build and this is still reproducible with e10s enabled. At step #5, the auto complete popup doesn't display. With non e10s at step #5 the auto complete popup is displayed.

Firefox: 46.0a2,  Build ID  20160209004008
User Agent  Mozilla/5.0 (Windows NT 6.3; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Firefox: 47.0a1, Build ID:  20160208030244
User Agent  Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Thanks,
Cosmin.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

The popup is rolled-up in the parent without informing the child. We need to update our child's state. With this patch, clicking on the input element causes the popup to close and then open again. That matches our non-e10s behavior and what Chrome does.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5f876f9ea0

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

The first patch failed on try because we could race with the child, re-opening the autocomplete popup before the child processed the message that we'd closed the popup, causing problems. This is ugly, but it works (in the parent, the popup manager handles the click and causes the rollup). We really need to separate autocomplete's handling of the popup and the DOM into the child and parent.

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04075c5cf3fd

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04075c5cf3fd

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I backed this patch out because of bug 1272172.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 49 but maybe the e10s team can suggest an owner to take this for 50.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

After some discussion of this in the Platform triage meeting and a possible dupe in bug 1300919 let's keep an eye on it for 49. Jimm asked that we also bump the priority back to P2. 
I don't think it blocks 49 but we may want to fix it in a dot release situation.

Comment 11

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: This is even more important in 51 since it now affects non-e10s too. See bug 1300919 comment 1.

Comment 12

[Gerry Chang [:gchang]]

Track for 51+ as regression.

Comment 13

[Andrew Overholt [:overholt]]

Matt, any chance you and the autofill team can take a look?

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

This seems to fix it for me in both e10s and non-e10s with the sample file provided, so I'm going to tentatively assign this to myself and test this some more tomorrow before sending for review. Clearing n-i for now.

Comment 15

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e08f8d6615

Comment 16

[Stephen A Pohl [:spohl]]

Comment on attachment 8797358 [details] [diff] [review]
Patch

I couldn't find any problems after testing this some more and try is green. The logic here is basically: If the popup is open and we get a MouseDown event, we always want to close the popup (instead of ignoring the event). This maintains the proper internal state of whether or not the popup is opened. Previously, the popup would get closed but the internal state did not reflect this.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8797358 [details] [diff] [review]
Patch

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

Hm I'm not sure I'm the best person to review this. I suggest MattN or Enn.

Comment 18

[Stephen A Pohl [:spohl]]

Comment on attachment 8797358 [details] [diff] [review]
Patch

(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> Comment on attachment 8797358 [details] [diff] [review]
> Patch
> 
> Review of attachment 8797358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm I'm not sure I'm the best person to review this. I suggest MattN or Enn.

Thanks! Forwarding to MattN.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8797358 [details] [diff] [review]
Patch

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

Redirecting to mconley since I have a review backlog and he refactored some autocomplete stuff recently therefore would probably have more context for this.

Comment 20

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

Comment on attachment 8797358 [details] [diff] [review]
Patch

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

This makes sense to me. I wonder why we didn't just do this in the first place.

Thanks spohl!

Comment 21

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b443899d03b49b3e0af52e9274b287c990798315
Bug 1183037: Ensure that autocomplete popups can be displayed again after being dismissed the first time. r=mconley

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b443899d03b4

Comment 23

[Gerry Chang [:gchang]]

Hi :spohl,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51?

Comment 24

[Stephen A Pohl [:spohl]]

Comment on attachment 8797358 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1294502 exposed this for both e10s and non-e10s (see bug 1300919 comment 1)
[User impact if declined]: Form autocomplete popups don't display anymore after being dismissed a first time.
[Describe test coverage new/current, TreeHerder]: verified locally
[Risks and why]: simple one-line change, should be very low risk.
[String/UUID change made/needed]: none

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8797358 [details] [diff] [review]
Patch

Fix a regression. Take it in 51 aurora.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29a2722ad461

Comment 27

[Brindusa Tot, Desktop QA]

Verified as fixed with e10s enabled and disabled, on
 - Nightly 52.0a1, Build ID 20161113030203, on Windows 7, Mac OS X 10.10 and Ubuntu 16.04.
 - Aurora 51.0a2, Build ID 20161114004005, on Windows 7, Mac OS X 10.10 and Ubuntu 16.04.

Comment 28

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

Too late to fix in 50.1.0 release