495437 - ContentClickingModule confuses the reader by using both _clickTimeout and clickTimeout

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marien Zwart]

It does this in one or two (depending on the version of the code) places:

  window.clearTimeout(this._clickTimeout);
  this.clickTimeout = -1;

The first time I read through this code and tried to understand why it worked this confused me, because I think if it assigned -1 to this._clickTimeout (which I thought it did) it would break doubleclick. The code is definitely not quite right since nothing ever reads this.clickTimeout, just this._clickTimeout. Minor because the code does seem to work in practice, it's just weird.

Comment 1

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

Hmm, initialy I thought there was a property named "clickTimeout", but I see none. Seems like a syntax error.

Comment 2

[Marien Zwart]

Well, I do not think "error" is the right word here. What currently happens for a doubleclick is:

Mouse down: event pushed.

Mouse up: event pushed, _clickTimeout set (because not set yet)

Mouse down (before _clickTimeout expires): event pushed, _clickTimeout cleared, but not set to -1 (clickTimeout is set to -1, but this has no effect).

Mouse up: event pushed, _clickTimeout cleared again, clickTimeout set to -1 again (still no effect), doubleclick sent.

Clearing the timeout in the second mousedown seems reasonable, but if that starts setting _clickTimeout to -1 too the _sendDoubleClick call in mouseup becomes unreachable. So I think that logic needs to change: perhaps mouseup should be checking for the number of events in _events instead? If there are 4 events in there (including this mouseup) it's a doubleclick, instead of checking if _clickTimeout is not -1 to decide if it is a doubleclick?

Comment 3

[Ben Combee (:bcombee)]

This code has been like this since the file was created.  I don't think there was even an intention to have a separate clickTimeout member, though.

Comment 4

[Ben Combee (:bcombee)]

Attachment: Change this.clearTimeout sets to this._clearTimeout

Comment 5

[Ben Combee (:bcombee)]

Comment on attachment 380461 [details] [diff] [review]
Change this.clearTimeout sets to this._clearTimeout

With this change, I can't double click anymore to zoom into an element.

Comment 6

[Ben Combee (:bcombee)]

Attachment: Try #2 - removed one set, fixed other, added comments

This one allows double click to stay working while still cleaning up the code.  Tested with Google News page

Comment 7

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

http://hg.mozilla.org/mobile-browser/rev/e774a44dbf60

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on builds (using timing scenarios with double-click and single-click on google news):

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

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre