635465 - JavaScript menu is not working on sports.ru

Status: RESOLVED FIXED

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

Description

[Yakove]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110218 Firefox/4.0b12pre
Build Identifier: 

Cannot go to the menu in the main site menu

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.sports.ru/
2. Put the cursor on the main menu item in the header, for ex. "ФУТБОЛ"
3. Try to click at one of the drop-down menu, it disappears
Actual Results:  
Submenu is disappears, cannot click on it

Comment 1

[Alice0775 White]

Regression window:
Works;
http://hg.mozilla.org/mozilla-central/rev/dbf0cee832f6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre ID:20110125102831
Fails:
http://hg.mozilla.org/mozilla-central/rev/273cb783edac
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre ID:20110125105431
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dbf0cee832f6&tochange=273cb783edac
Regressed by:
273cb783edac	Kevin Gadd — bug 627628 - Throttle the dispatching of starved paints so that time is allowed for the processing of input events between each starved paint. This helps maintain responsiveness in cases where input events are being generated very rapidly or where painting is extremely expensive. r=roc a=roc

Comment 2

[Alice0775 White]

(FYI, This happens on Ubuntu too, however regression window is different. The problem happens on Namoroka/3.6.15pre and Shiretoko/3.5.18pre )

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #1

It looks like we forget about an element's hover state when it's reframed? The test works if I remove the "item.offsetWidth" line.

Comment 4

[Mats Palmgren (inactive)]

I don't think this needs to block since it also occurs in Fx 3.6 and 3.5,
at least on Linux.

Comment 5

[Timothy Nikkel (:tnikkel)]

This is probably the same as bug 592954, comment 36 and comment 38 explain the problem.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to comment #4)
> I don't think this needs to block since it also occurs in Fx 3.6 and 3.5,
> at least on Linux.

Linux is different in this case, because it has never dispatched starved paints.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

tn: so, does this need to block?

Comment 8

[Timothy Nikkel (:tnikkel)]

Do the dropdown menus in bug 592954 and bug 592093 work currently in Windows? aka is this menu a special case or did this whole class of drop downs regress?

Comment 9

[Alice0775 White]

Before landing bug 627628
They work on Windows7 properly.

After landing bug 627628
They work on Windows7 unwillingly. It need moving mouse _very_ _very_ slowly.

So, I think that bug 627628 is something wrong. this bug is not special case.


http://hg.mozilla.org/mozilla-central/rev/657c2a92ee2b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110219

Comment 10

[Alice0775 White]

Comment #9 is a reply for comment #8

Comment 11

[Timothy Nikkel (:tnikkel)]

So to fix this we either need to back out bug 627628 or fix the general problem here (see comment 5) in a better way, althrough I'm not sure what that is. Note that the original reason for taking bug 627628 (slow twitter typing) is now fixed by twitter taking my suggestion to avoid the problem on their site.

I'm thinking this might block.

Comment 12

[Timothy Nikkel (:tnikkel)]

So far the best idea I've come up with is: if we flush sometime during the processing of an event, then flush after we have completely processed the event. But that does not seem like something we want to take this late in the game.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Let's do the backout, and fast.

Comment 14

[Timothy Nikkel (:tnikkel)]

Backed out bug 627628 to fix this bug
http://hg.mozilla.org/mozilla-central/rev/88afdccd3ba6

Comment 15

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tn) from comment #12)
> So far the best idea I've come up with is: if we flush sometime during the
> processing of an event, then flush after we have completely processed the
> event. But that does not seem like something we want to take this late in
> the game.

I'm not sold on the "flush afterwards" strategy, though it might be the best compromise considering the performance implications of flushing too often.

Conceptually, flushing should be something that's done *before* a crucial point. And in this case that point seems to be when we search for the element under the mouse in order to know where to dispatch the event to; this bug happens because at that point we're looking at outdated layout information.
So I think it would make most sense to flush right before looking for the event target. And I think it would make sense to do that for all positioned events.

What do you think about that?

Comment 16

[Timothy Nikkel (:tnikkel)]

You're right that flushing before finding the target of a positioned event makes the most sense. And like you mention I am worried that flushing on every mouse move might cause performance problems. smaug, what do you think? (comment 5 if you need to refresh your memory about the problem here)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Logically, flushing before we dispatch positioned events is the right thing to do.

Maybe we should just try it and see. Maybe avoiding those flushes is a premature optimization. Boris, what do you think?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If it causes problems, one way to deal with it might be (and this might be a crazy idea) to flush before all positioned events except for mouse moves, and dispatch mouse-move events off the refresh driver (after flushing). That would ensure flushes for paints are coalesced with flushes for mouse-move events, and that mouse-move events are throttled.

And maybe we'd need something similar for touches.

Comment 19

[Boris Zbarsky [:bzbarsky]]

We used to flush on every mousemove (e.g. see nsEventStateManager on the aviary branch; the NS_MOUSE_MOVE case in PreHandleEvent); the claim in the comments was that we wanted that to avoid flicker; that was done to fix bug 36849.  We took it out in bug 244366 when we came up with a better solution for dealing with the flicker.

I could probably live with reintroducing it, esp. if we do some careful measurement (and e.g. optimize the no-op flush case to be faster as needed).

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I actually kinda like the idea of firing mousemoves in the refresh driver.

Optimizing no-op flushes is a good idea no matter what!

Comment 21

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

Firing mousemoves when? After refreshing everything else? Or before?

(I don't know in which order different things run on refreshing driver.)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Olli Pettay [:smaug] from comment #21)
> Firing mousemoves when? After refreshing everything else? Or before?

After (and then flush again before painting).

Comment 23

[Markus Stange [:mstange]]

Attachment: flush before positioned event

This fixes the sports.ru testcase, but it doesn't fix the testcase in bug 592954.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I just tried writing a test for this, but it seems to fail randomly even on trunk (without the patches in bug 598482), at about the same frequency as in that bug.

We really need to figure out a way to test this....

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, and we optimized no-op flushes a bit in 709256.  At this point they're under 150ns each on my hardware.