1312244 - [e10s] autocomplete popup does not close

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[blinky]

Steps to reproduce:

1. Go to https://dl.dropboxusercontent.com/u/95157096/85f61cf7/rwozgxgjke.html.
2. Click on the <input> element to open.
3. Click on the <input> element to close.


Actual results:

autocomplete popup does not close.

Comment 1

[blinky]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cf7649531c81d88c1948c88512b0c8c50cf0f2e3&tochange=4b2fef1aa33e310f577dbe36a6c6abf3f1c7f0ce

Regressed by: bug 1296638

Comment 2

[Loic]

Attachment: rwozgxgjke.html

Comment 3

[Loic]

[Tracking Requested - why for this release]:

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(Copying the HTML from Dropbox to URL field)

Comment 5

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

Interestingly, this only seems to be true with e10s enabled.

Comment 6

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

Okay, I've figured out what's going on here.

The popuphiding / popuphidden events are fired synchronously when the popup manager decides we're doing a "rollup" (closing the popups due to "outside" events). In this case, the "outside" event is a click on the input element in content.

Here's the kicker though - we handle that popuphidden event and queue a message to content _before_ the mouse event that caused the roll-up is dispatched to content. This means that browser-content.js hears that the popup closed due to popuphidden first, sets its internal state of popupClosed to true, and then the MouseDown on the input field is processed. The nsFormFillController then queries the popup, sees its closed, and tells the parent to open it again.

My fix for this relies on the work in bug 1183037, which has the nsFormFillController tell the popup to close on MouseDown if it's already open.

I'm adding the norolluponanchor attribute (first introduced in bug 1116865) on the autocomplete popup panel, which causes us to ignore the rollup event if it happens to be targeted within the anchor rect. This means that we need to trust content to tell us to close the autocomplete popup if the input is clicked when the popup is open (and this is what bug 1183037 does).

Comment 7

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

Attachment: Bug 1312244 - Autocomplete popup does not close when anchor clicked a second time.

The popuphiding / popuphidden events are fired synchronously when the popup manager
decides we're doing a "rollup" (closing the popups due to "outside" events). In the case
of this bug, the "outside" event is a click on the input element in content.

Here's the kicker though: we handle that popuphidden event and queue a message to content
_before_ the mouse event that caused the roll-up is dispatched to content. This means that
browser-content.js hears that the popup closed due to popuphidden first, sets its internal
state of popupClosed to true, and then the MouseDown on the input field is processed. The
nsFormFillController then queries browser-content's AutoCompletePopup, sees its closed,
and tells the parent to open it again.

This patch adds the norolluponanchor attribute to the panel, which causes us to ignore the
rollup event if it happens to be targeted within the anchor rect. This means that we need
to trust content to tell us to close the autocomplete popup if the input is clicked when
the popup is open (and this is what bug 1183037, which this bug depends on, does).

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

Comment 8

[Neil Deakin]

Comment on attachment 8803907 [details]
Bug 1312244 - Autocomplete popup does not close when anchor clicked a second time.

https://reviewboard.mozilla.org/r/88118/#review87056

::: toolkit/components/satchel/AutoCompletePopup.jsm:183
(Diff revision 1)
> +      // mouse event occurs on the anchor, it will think that the popup is
> +      // closed and might attempt to open the popup again.
> +      //
> +      // So if the user clicks on the anchor, the parent ignores the rollup, and
> +      // we trust content to tell us to close the popup instead.
> +      this.openedPopup.setAttribute("norolluponanchor", true);

There is always only one and only one popup this will apply to? Can the attribute instead be set in the markup?

Comment 9

[Marcia Knous [:marcia]]

Tracking 52+ - popup should close, recent regression that should be addressed.

Comment 10

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

Comment on attachment 8803907 [details]
Bug 1312244 - Autocomplete popup does not close when anchor clicked a second time.

https://reviewboard.mozilla.org/r/88118/#review87056

> There is always only one and only one popup this will apply to? Can the attribute instead be set in the markup?

Yeah, can do. I guess I was trying to avoid breaking SeaMonkey for this case, but I'll just file a bug and let them know what they need to do instead.

Comment 11

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

Comment on attachment 8803907 [details]
Bug 1312244 - Autocomplete popup does not close when anchor clicked a second time.

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

Comment 12

[Neil Deakin]

Comment on attachment 8803907 [details]
Bug 1312244 - Autocomplete popup does not close when anchor clicked a second time.

https://reviewboard.mozilla.org/r/88118/#review87314

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38ac5c0d5d55
Autocomplete popup does not close when anchor clicked a second time. r=enndeakin+6102

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/38ac5c0d5d55

Comment 15

[Vuyisile Ndlovu [:terrameijar]]

Verified fixed on Beta 52.0b8 with e10s enabled and Nightly 54.0a1 (e10s disabled) on Ubuntu Linux

The popup closes as expected.