Closed Bug 1183037 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Widget, defect, P2)

41 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
e10s + ---
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 + verified
firefox52 --- verified

People

(Reporter: mantaroh, Assigned: spohl)

References

Details

(Keywords: regression, Whiteboard: tpi:+)

Attachments

(2 files, 2 obsolete files)

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.
tracking-e10s: --- → ?
Summary: Once displayed, Autocomplete popup will not display after the second time. → [e10s]Once displayed, Autocomplete popup will not display after the second time.
Assignee: nobody → mrbkap
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.
Priority: -- → P1
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #8746842 - Flags: review?(felipc)
Attachment #8746842 - Flags: review?(felipc) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
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
Attachment #8747975 - Flags: review?(MattN+bmo)
Attachment #8747975 - Flags: review?(MattN+bmo) → review?(felipc)
Attachment #8747975 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/04075c5cf3fd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272172
I backed this patch out because of bug 1272172.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: P1 → P3
Whiteboard: tpi:+
Too late for 49 but maybe the e10s team can suggest an owner to take this for 50.
Duplicate of this bug: 1300919
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.
Priority: P3 → P2
Assignee: mrbkap → nobody
[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.
Status: REOPENED → NEW
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: [e10s]Once displayed, Autocomplete popup will not display after the second time. → Once displayed, form autocomplete popup will not display after the second time
Track for 51+ as regression.
Matt, any chance you and the autofill team can take a look?
Flags: needinfo?(MattN+bmo)
Target Milestone: mozilla49 → ---
Attached patch PatchSplinter Review
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.
Assignee: nobody → spohl.mozilla.bugs
Attachment #8747975 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
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.
Attachment #8797358 - Flags: review?(felipc)
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.
Attachment #8797358 - Flags: review?(felipc)
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.
Attachment #8797358 - Flags: review?(MattN+bmo)
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.
Attachment #8797358 - Flags: review?(MattN+bmo) → review?(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!
Attachment #8797358 - Flags: review?(mconley) → review+
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
https://hg.mozilla.org/mozilla-central/rev/b443899d03b4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :spohl,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51?
Flags: qe-verify+
Flags: needinfo?(spohl.mozilla.bugs)
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
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8797358 - Flags: approval-mozilla-aurora?
Comment on attachment 8797358 [details] [diff] [review]
Patch

Fix a regression. Take it in 51 aurora.
Attachment #8797358 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Too late to fix in 50.1.0 release
You need to log in before you can comment on or make changes to this bug.