118877 - [viewpoint] Plugin adjustCursor handler causes mad cursor flicker due to NS_MOUSE_MOVE handling conflict

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Jonathan Brecher]

From Bugzilla Helper:
User-Agent: Mozilla/4.75 [en] (Windows NT 5.0; U)
BuildID:    20011213

We have a plugin that uses handles the adjustCursor event to set the cursor 
(when the cursor is within the bounds of the plugin).  Our handler sets the 
cursor correctly, but it is then immediately reset to the pointer.  The problem 
appears to be as follows:

PresShell::HandleEventInternal() calls manager->PreHandleEvent() prior to 
calling targetContext->HandleDOMEvent(), where HandleDOMEvent passes the event 
to my plugin.  But PreHandleEvent takes a NS_MOUSE_MOVE event and changes the 
cursor unconditionally in that case.  That same NS_MOUSE_MOVE event in turn gets 
passed to my plugin as an adjustCursor event, which sets the cursor back.  Etc.

Reproducible: Always
Steps to Reproduce:
1. Load plugin that changes the cursor in response to an adjustCursor event
2. Wave mouse in plugin

Actual Results:  Cursor flickers between desired image and the pointer

Expected Results:  Cursor should remain at the desired image as set in the 
adjustCursor handler

I've marked the severity high because this is a major usability issue for our 
plugin, and our users are going to have fits if it isn't changed.

Please contact me if you need a copy of our plugin for testing purposes.  
Hopefully it'll be in stable shape by the time you ask...

Comment 1

[Peter Lubczynski]

This sounds similar to a problem Ari at Viewpoint was having, but with
windowless plugins.

Reporter: Can you try an updated, newer build?

Comment 2

[Jonathan Brecher]

No obvious change in 20020109 (source code from ~11 pm last night)

Comment 3

[Ari Berger]

Peter is right- we are having the same problem.  we are a windowless plugin. In 
PreHandleEvent() does the following on mousemove:
    GenerateDragGesture(aPresContext, (nsGUIEvent*)aEvent);
    UpdateCursor(aPresContext, aEvent, mCurrentTarget, aStatus);
    GenerateMouseEnterExit(aPresContext, (nsGUIEvent*)aEvent);
    // Flush reflows and invalidates to eliminate flicker when both a reflow
    // and visual change occur in an event callback. See bug  #36849
    FlushPendingEvents(aPresContext); 

UpdateCursor changes the cursor to an arrow.
FlushPendingEvents sends the event to our plugin, where we change the cursor.
Let me know if you need plugin and testcase.

Comment 4

[Ari Berger]

Should I open a separate bug for PC, or is this enough?

Comment 5

[Peter Lubczynski]

cc:ing some more folks

Comment 6

[Brian Nesse (gone)]

Wow! we have so many cursor bugs I think we need a meta bug to track them all.
:) Hmm, see bug 61955, bug 104970, bug 93145, and... there's one more that both
beard and I commented in which I can't find right now. As I recall, the general
idea of the comments was that when the cursor was over the plugin, we should not
be touching it.

Comment 7

[Peter Lubczynski]

This kinda sounds like 104970. The cursor flickering is important to Viewpoint.

Ari, can you attach a Viewpoint testcase showing the problem? Thanks!

Comment 8

[Ari Berger]

I will attach a test case. Problem is, in our most recent plugins, we disabled 
changing the cursor to avoid the flicker.  I will do a build with it reenabled 
and attach it tomorrow.

Comment 9

[Michael Dunn]

Peter, What is the next step here; Is this still needed for 094? My read of
Ari's comment is that Viewpoint has a workaround implemented.

Comment 10

[Ari Berger]

No- we had no workaround.  We just disabled the feature.
As I said, the cursor flickers when the content changes the cursor from arrow 
to something else. We disabled that feature for the CS browser.  We really want 
to reenable it before ship.
I will have a simple test case and plugin with feature enabled this afternoon.

Comment 11

[Ari Berger]

Here is a zipped installer:
C:\Documents and Settings\aberger\Desktop\install\VMPInstaller.zip

It contains an executable installer to run.  If you are using a debug browser 
so the npViewpoint.dll and .xpt don't install into your browser, they are 
included too. Put them in the plugins (not components) folder.
 
Note that this is not our standard installer- it has some debug stuff (like 
dirty rects) enabled.  It may even crash.  It's ok- it's not the shipping 
version.

Here is a test case:
http://cole.viewpoint.com/~compuserve/hidden/cursortest/index.html
Drag the mouse around.  It will probably flicker. It shouldn't.  If you can't 
repeat, please let me know.

Comment 12

[Ari Berger]

sorry: pasted the wrong installer path. you can't get that one...
http://cole.viewpoint.com/~aberger/VMPInstaller200.zip

Comment 13

[Michael Dunn]

I would consider the disabled feature in fact a workaround for the failure if
the plugin can in fact ship with the feature disabled. In otherwords is this a
true stopper?

Comment 14

[Stew Rubenstein]

We consider this a blocking bug, as the cursor feedback is critical in our 
plugin to help the user to know what will happen when they click.  The 
workaround of not ever trying to set the cursor is not acceptable.

Comment 15

[Ari Berger]

For the viewpoint plugin, this was not marked as a blocker, just high priority.

Comment 16

[Peter Lubczynski]

-->peterl

Comment 17

[Peter Lubczynski]

Attachment: possible fix?

With this patch, my cursor doesn't flicker on Ari's testcase on Windows. Still
need to test Mac.

Reporter, do you have a testcase? I'll try those other bugs...

Comment 18

[Jonathan Brecher]

Quick check on Mac looks very good!

Comment 19

[Ari Berger]

Patch looks great! We will reenable the feature. Thanks Peter.

Comment 20

[Michael Dunn]

Plussing per verification of fix

Comment 21

[Brian Nesse (gone)]

The patch looks ok but I wonder if, instead of returning an error, you should
instead declare a new cursor type... i.e.

         aTargetFrame->GetCursor(aPresContext, aEvent->point, cursor);
+        if (cursor == NS_STYLE_CURSOR_OVERRIDE)
+          return;  // this frame (probably a plugin) is managing it's own cursor

This seems more obvious to me, but I will gladly defer to someone with more
events experience.

Comment 22

[Kevin McCluskey (gone)]

Comment on attachment 66232 [details] [diff] [review]
possible fix?

Patch looks fine to me. I think the comment could be a little clearer. It
should clearly state that it consumes the event when GetCursor fails. 

r=[e-mail]

Comment 23

[Peter Lubczynski]

*** Bug 112797 has been marked as a duplicate of this bug. ***

Comment 24

[Michael Dunn]

Need super review on this and also on bug 120821 (P1, viewpoint painting problem
in absolute DIV/scrolling) and then I can land right away. I sent mail yesterday
to [e-mail] and Patrick but I have not heard back yet. Maybe today?

Comment 25

[Michael Dunn]

I need super review on this and then I can land right away. I sent mail
yesterday to [e-mail] and Patrick but I have not heard back yet.
Maybe today?

Comment 26

[Patrick C. Beard]

Comment on attachment 66232 [details] [diff] [review]
possible fix?

sr=beard

Comment 27

[Michael Dunn]

Plusing

Comment 28

[Peter Lubczynski]

fixed0.9.4

Comment 29

[Peter Lubczynski]

in trunk

Comment 30

[shrirang khanzode]

used Ari's testcase/installer. Mouse cursor does not flicker, this works fine. 
verif on 0.9.4 branch build.(01/28)

Comment 31

[shrirang khanzode]

.