Mantaroh Yoshinaga[:mantaroh] Reporter
	Description • 9 years ago

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.

	Cosmin Muntean [:cmuntean], Ecosystem QA (PTO until September 8th)
	Comment 1 • 8 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 3 • 8 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 4 • 8 years ago

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

	Pulsebot
	Comment 5 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 6 • 8 years ago
bugherder

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 7 • 8 years ago

I backed this patch out because of bug 1272172.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 8 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

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.

	Matthew N. [:MattN]
	Comment 11 • 8 years ago

[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.

	Gerry Chang [:gchang]
	Comment 12 • 8 years ago

Track for 51+ as regression.

	Andrew Overholt [:overholt]
	Comment 13 • 8 years ago

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

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 14 • 8 years ago

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.

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 15 • 8 years ago

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

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 16 • 8 years ago

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.

	:Felipe Gomes (needinfo for replies!)
	Comment 17 • 8 years ago

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.

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 18 • 8 years ago

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.

	Matthew N. [:MattN]
	Comment 19 • 8 years ago

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.

	Mike Conley (:mconley) (:⚙️)
	Comment 20 • 8 years ago

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!

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 21 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 22 • 8 years ago
bugherder

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

	Gerry Chang [:gchang]
	Comment 23 • 8 years ago

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

	Stephen A Pohl [:spohl] (OOO until 9/8) Assignee
	Comment 24 • 8 years ago

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

	Gerry Chang [:gchang]
	Comment 25 • 8 years ago

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

Fix a regression. Take it in 51 aurora.

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder uplift

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

	Brindusa Tot, Desktop QA
	Comment 27 • 8 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 8 years ago

Too late to fix in 50.1.0 release