493251 - Header/navigation links on PG.com take two clicks to load

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre

The top navigation/header links on http://pg.com/en_US/index.shtml, such as "Products", etc. take a double-click to load -- in 3.0.10 they require only a single click.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This regressed between the 2009-02-26-02 (rev a979ac23e17e) nightly and the 
2009-02-27-02 (rev cc8d4e656113) nightly.

Pushlog:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a979ac23e17e&tochange=cc8d4e656113

Comment 2

[Timothy Nikkel (:tnikkel)]

Most likely bug 333198 Suppress Input events for web content during synchronous XMLHttpRequest loads (http://hg.mozilla.org/mozilla-central/rev/7184f7d69ff0).

pg.com fires off a sync XHR on every click of a link to do click tracking.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, I was wondering about that.

smaug, how come this xhr that's dispatched off the click is affecting the anchor getting the click?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I'll look at this tomorrow.

Comment 5

[Johnny Stenback (:jst)]

Recent regression that could show up on a number of sites it seems. Blocking.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> smaug, how come this xhr that's dispatched off the click is affecting the
> anchor getting the click?
It is not click, but mousedown/up. If I read the code right, the site does
sync XHR during mousedown, and that prevents us to react to mouseup (which would then cause click to be fired). 
If one release mouse pretty slowly, the click is generated.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This generalizes delayed event queue, so that it may contain also mouseup and keyup/press events.
New mousedown-up-click or keydown-press-up sequences aren't allowed, only those 
ones which aren't finished when sync XHR starts.

I must still write mochitests for this, but feel free to review.
A test for keyevent http://mozilla.pettay.fi/key_and_sync_xhr.html

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> A test for keyevent http://mozilla.pettay.fi/key_and_sync_xhr.html
Btw, Opera and Webkit queues all events, though webkit messes the event ordering
pretty badly.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 377856 [details] [diff] [review]
patch

+        if (aEvent->message == NS_KEY_DOWN) {
+          mNoDelayedKeyEvents = PR_TRUE;

Would this not make more sense if mNoDelayedKeyEvents was named mDelayedKeyEvents, and the logic was flipped around here? Same for mNoDelayedKeyEvents?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I don't see much difference between mNoDelayedKeyEvents and
!mDelayedKeyEvents, though actually mNoDelayedKeyEvents may make more sense
because that is a flag which explicitly must be set to prevent delayed events.
(::new initializes everything to 0/PR_FALSE)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 377856 [details] [diff] [review]
patch

I didn't understand enough of the patch before commenting about the names. This patch looks good. r+sr=jst

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with tests

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/3934f51e350f

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92c7854d4192

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090519 Minefield/3.6a1pre; will verify on branch when the next Shiretoko nightly is out.

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Also verified FIXED on the 1.9.1 branch, using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre (.NET CLR 3.5.30729).

Replacing fixed1.9.1 keyword with verified1.9.1.