515789 - Awesome bar pops up when click on form autocomplete fields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2a2pre) Gecko/20090910
Fennec/1.0a3

and

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090910
Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2a2pre) Gecko/20090910
Fennec/1.0b4pre

Steps to Reproduce:
1. Go to www.gmail.com and wait for it to load and have the username field highlighted/focused
2. Click on the username field.

Actual Results:
The awesome bar will pop up 

Expected Results:
The awesome bar should not pop up.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

This is not limited to gmail.

I saw this happen when I went to http://w3c.org, and click in the left top of page.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

about:config does this as well for the checkbox and the let me in button.  no way to use about:config anymore.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This seems to work but need a bit more testing, though.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The same but expressed in an other manner and with clean up of (now) unneeded part of InputHandler.js

Comment 5

[Roy Frostig :froystig]

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

>-  /**
>-   * Clear all recorded events.  This will *not* automagically redispatch any
>-   * pending un-redispatched events.  If you desire to redispatch everything
>-   * in the recorded events buffer, you should call _redispatchDownUpEvents()
>-   * before calling _clearDownUpEvents().
>-   *
>-   * @param [optional] the number of events to remove from the front of the
>-   * recorded events queue.
>-   */
>-  _clearDownUpEvents: function _clearDownUpEvents(howMany) {
>-    if (howMany === undefined)
>-      howMany = this._downUpEvents.length;
>-
>-    this._downUpEvents.splice(0, howMany);
>-    this._downUpDispatchedIndex = Math.max(this._downUpDispatchedIndex - howMany, 0);
>-  },

You want to keep this function, as you'll need to use it.  See following comment.


Suppose that this does *not* happen (i.e. the common case where commitToClicker is true):

>+    else
>       this._cleanClickBuffer();    // clean the click buffer ourselves, since there was no clicker
>                                    // to commit to.

Then the _commitAnotherClick in the if clause before this else clause will set a timeout (via setTimeout), which will eventually call _doSingleClick or _doDoubleClick, which will call _cleanClickBuffer on their own.  Within that timeout period, the InputHandler may already get a new MouseDown event, which will cause it to _recordEvent that event.

Now, _doSingleClick was doing

>-    this._cleanClickBuffer(2);
>+    this._cleanClickBuffer();

and _doDoubleClick was doing

>-    this._cleanClickBuffer(4);
>+    this._cleanClickBuffer();

because _cleanClickBuffer was taking that number as its howMany parameter and forwarding it to:

>-    this._clearDownUpEvents(howMany);

In place of which, this patch does:

>+    this._downUpEvents = [];

In the old case, howMany will make _clearDownUpEvents only splice out the events recorded in the MouseDown/MouseUp event group corresponding to this single/double click.  If you don't use _clearDownUpEvents with howMany, and instead just reset _downUpEvents to an empty array, you are also wiping out any events that may have been recorded during the timeout period started by _commitAnotherClick, which usually means you lose your MouseDown event.  Now your _downUpEvents array is smaller than expected, and MouseModule will throw array-item-undefined exceptions on the next time that something like:

>     let ev = this._downUpEvents[1].event;
>     this._clicker.singleClick(ev.clientX, ev.clientY);

is executed (similarly in the double-click case).

So that's a long explanation for the small detail as to why that function is still necessary and still needs to be called.  It's a tricky race condition.


Other changes look fine.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

(In reply to comment #5)
> (From update of attachment 400363 [details] [diff] [review])

> So that's a long explanation for the small detail as to why that function is
> still necessary and still needs to be called.  It's a tricky race condition.

argh! :)

Comment 7

[Ben Combee (:bcombee)]

Comment on attachment 400449 [details] [diff] [review]
Patch v0.2

I'm glad to see that code simplified.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 400449 [details] [diff] [review]
Patch v0.2

Waiting for froystig to review before I push

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/7588e2aa35d4

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2a2pre) Gecko/20090915 Fennec/1.0a3

and

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090915
Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090915
Fennec/1.0b4pre