76053 - Windows mouse integration: "Snap to default button in dialog boxes"

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[gekacheka]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; 0.8.1) Gecko/20010323
BuildID:    2001032319 

Windows mouse control panel has option 
"Snap to default: move pointer to default button in dialog boxes"
But Mozilla dialogs do not implement this, making them tedious to use, e.g.,
each time clicking on the NEXT button pops up a confirmation dialog (ok to go to
next folder).

Reproducible: Always
Steps to Reproduce:

In windows "mouse" control panel, under tab "Motion", make sure "Snap to
Default" is checked (on).  

Popup a Mozilla dialog.  Any dialog will do.

e.g., If you have filtered mail, click on the next button; every time you switch
to next folder, it puts up a confirmation dialog, and have to drag mouse to the
dialog each time to click ok.

Or select the "Sent" folder and click "DELETE" (Trash icon).  A dialog will pop
up, but have to move mouse to click on either button.

Actual Results:  Dialog pops up, but pointer does not move, so you have to drag
pointer to dialog to click ok.

Expected Results:  Dialog pops up, and pointer is repositioned over the default
button of the dialog ("ok" in a confirmation dialog).

Comment 1

[Warner Young]

Hmm, my first thought was that Mozilla dialogs weren't true Windows dialog 
classes, but apparently they are!  That probably means the dialogs aren't 
properly implementing the DM_GETDEFID message or something similar.  But either 
way, snapping to the default dialog isn't a function of the dialog, it's 
actually done by the mouse driver.

Comment 2

[timeless]

yeah...

Comment 3

[Peter Trudelle]

->future

Comment 4

[Keyser Sose]

Marking NEW.

Comment 5

[Ashley Bischoff (blog at handcoding.com)]

Updated summary to include search term "mouse".

Comment 6

[Jim Song]

I have made an Windows testing program, which override 
the DM_GETDEFID processing. 
I found this message has never been received, untile it 
was sent by myself.
Anyway, from this test program, I conclude that this message
is not related to the default button snap. 

Can Warner tell me why do you say it's the functioning 
of mouse driver?


While I am checking nsWindow::WindowProc, I found this 
message is received every time we switch back to the 
preference window. It seems this message has not be
processed by the Window system.( maybe by XUL ? )

This is an accessibility issue, since we should never
disable or block the system's Accessibility Function.

Comment 7

[Warner Young]

Jim, I'm not saying DM_GETDEFID is the problem.  I was just suggesting it
*might* be.  I haven't looked in detail at the "snap to default button" option,
so I don't know how it works.  DM_GETDEFID seemed like a good guess, because
that's how you're supposed to find out which button is the default one.

Comment 8

[Jim Song]

Hi, Warner,
Things turn clear to me now!

In my testing program, I block the default message handler for
WM_SHOWWINDOW, ie. CDialog::OnShowWindow, then my standard MFC
test program would not snap to default button any more. So I 
think we should do the same thing as OnShowWindow does.

Then I search the MSDN, and found the SDK function SystemParamertersInfo,
which could provide us whether or not the SnapDefault attribute
is set. I think what we do is to move the cursor to default button
if the feature is set when the dialog is first updated.

I would provide a patch following upper idea. How do you think 
about it?

Comment 9

[Warner Young]

Jim, I'm not in charge of this bug and I don't work on the code at all, so I'm
not the one to decide what to do.  However, I would say that checking
SystemParametersInfo and handling this behavior in Mozilla is a bad idea, unless
that's the only way to do it.  In theory, the system should be doing this for
us, without any extra work on our part.

Comment 10

[Dan M]

We don't use system button controls, so the mouse driver doesn't understand that
there's a button to be snapped to. Reassigning to the accessibility guys, who
may have a clue what to do with this.

Comment 11

[Jim Song]

I think this feature is done by the software, 
but not by the Windows system.

From my test program, I found it is finished by
the MFC function CWnd::OnShowWindow. But we
didnot use MFC, so we have to do it ourselves.

Comment 12

[Aaron Leventhal]

This reminded me to file bug 129606 to support STATE_DEFAULT. With that we will
at least expose which button is default. Also filed bug 129605 to support
ROLE_DIALOG, so that we expose when a window is a dialog.

I hoped those would help, but they didn't.

Comment 13

[Aaron Leventhal]

Does anyone know how we find out if the snap-to default button setting is set in
Windows? Then we could do this ourselves.

Comment 14

[Ashley Bischoff (blog at handcoding.com)]

Aaron: 

Key: [HKEY_CURRENT_USER\Control Panel\Mouse]
Value Name: SnapToDefaultButton
Data Type: REG_SZ (String Value)
Value Data: (0 = disabled, 1 = enabled)

Due credit: http://www.winguides.com/registry/display.php/829/

Comment 15

[Jim Song]

Aaron,
Are you looking for the place to do the snapping work, 
or just about where the setting is?
You can find the setting in ControlPanel\Mouse\Moving.
(On Window2000)

And do you mean that bug 129606 and bug 129605 is dependent
on this one?

Comment 16

[Warner Young]

Aaron, the registry entry Alex gives will only work for Win2K and above, I
suspect.  At least, it doesn't seem to do anything on Win98.  It seems to me
there are 2 ways to do this:

1) Implement this functionality in Mozilla and use whatever appropriate API to
check for the state of this setting.  But this will only work on Win2K/XP.

2) Implement enough stuff to make Mozilla dialogs look like dialogs to Windows,
and the buttons to look like real buttons (your bug 129606 and bug 129605). 
This, I think, will make it work on Win2K/XP, as well as any special mouse
drivers (such as Logitech's) or 3rd-party utilities that implement this behavior
in the driver.

Comment 17

[Ashley Bischoff (blog at handcoding.com)]

Warner: Did you logoff and on again after setting the registry key? I'm not
disagreeing with your assessment, but I just wanted to check on that.

(http://www.windows2000faq.com/Articles/Index.cfm?ArticleID=13613)

Comment 18

[Aaron Leventhal]

Jim, bug 129605 and bug 129606 are independent of this one. I only mentioned
them to show this bug reminded me to file those. I thought exposing ourselves as
buttons via MSAA should have helped, but it doesn't.

Comment 19

[Warner Young]

Alex: yes, I did a full restart, and it didn't do anything.  I'm fairly sure
this functionality isn't built directly into Win 9x.

Comment 20

[Warner Young]

Aaron: I just did some quick testing on Win2K using one of my own apps.  It
looks like what the system is looking for is a button that has the
BS_DEFPUSHBUTTON style .  That's all it took.  I didn't have to play around with
messages or anything.  So in theory, as long as the Mozilla buttons *look* like
buttons to Windows, and if the default button has BS_DEFPUSHBUTTON style, then I
think everything will work automagically.

Comment 21

[Aaron Leventhal]

Right now our buttons in Windows don't have their own Window, we just draw them
on the screen. In standard controls each button has it's own window handle, correct?

I don't know that much about native Windows coding - where would you set this
style? Can it be done through something like a window class?

Comment 22

[Warner Young]

Aaron: ack!  I didn't realize Mozilla buttons were handled that way.  You're
correct.  In Windows, each dialog and dialog control is a separate window.  The
style can be set on any window, but I suspect (for this problem) that Windows
will look specifically for a control with the window class "Button" with the
specified style.

In that case, it may be that the only way to handle this would be through
Mozilla itself, which would bring up the problems I mentioned under item #1 in
comment 16.

Comment 23

[Aaron Leventhal]

Well, there would be another way.
We could create a window for every button in Mozilla - at least every default
button.

We already create a native window for combo boxes and list boxes. I suppose we
do that to get native scroll bars.

I think that's handled somewhere in widget/src/windows, but I haven't narrowed
down exactly where/how.

Comment 24

[Jim Song]

Attachment: Patch Prototype, Need Help

Hi, Aaron,
I make some testing in mozilla code and got some result.
I place some code in the nsButtonBoxFrame::HandleEvent,
there I can get the snapToDefault setting, and then 
move the cursor to somewhere. The code seems like a 
prototype to fix this bug, but I got many problems:
1) I need a button event such as WM_SHOWWINDOW in Windows
   to drive the added code. But I am not sure there is such
   an event, which would be sent to the buttonFrame when
   the dialog is first shown.
2) I don't know how to translate logic coordinate to
   screen coordinate.
3) I don't know how to move cursor in an platform-independent
   way.
So could you help me?

Comment 25

[Aaron Leventhal]

CC'ing danm, who is a better person for guidance on this bug.

In order to get the "snap to default button"  setting, a getter should be added
to nsIDeviceContext (gfx/public/nsILookAndFeel.h). This would be implements in
the platform specific nsLookAndFeel.cpp files, in the windows-specific one
you'll be able to use SPI_GETSNAPTODEFBUTTON contant in your call.

You can hook into new window creation in various ways. I might suggest tapping
into dialog.xml, and calling your routine from there. All <dialog> boxes use
dialog.xml to define their behavior. Also, that way you can do some of the work
in javascript, which is a bit easier. Here's the file:
/xpfe/global/resources/content/bindings/dialog.xml 

Both the accessibility code and the DOM Inspector both have to get the coords
relative to a screen. There's no pretty way to do it. You have to walk up the
frame hierarchy, adding relative bounds, until you get to a view or a widget.
You would use the widget if you wanted screen coords. Look at
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/base/src/inLayoutUtils.cpp#161
Don't forget to take advantage of GetElementsByAttribute(), if you need it.

CC'ing Pete Collins, who is dealing with content node x,y coordinates in another
bug. This functionality really needs to be put in a public interface where
everyone can use it, so we can have some better code reuse.

I also don't know how to move the mouse pointer, but I'm quite sure there's no
platform independant way. Mike Pinkerton says the best place to add a method is
nsIWidget.idl

Hope this helps. Good luck.

Comment 26

[:deanis74]

On Windows you use SetCursorPos().

BOOL SetCursorPos(
    int X,	// horizontal position  
    int Y 	// vertical position
   );

Comment 27

[Aaron Leventhal]

I won't get to this. Jim, do you still want it? 

Comment 28

[Gilbert Fang]

oh, jim has left our team for some time. I am not sure whether he will work on
mozilla now. Before he left, he passed this bug to me. But to tell the truth, I
am not confident for fixing this bug because this is only a window feature and I
am not familiar with it at all now. Anyway, I will own this bug before any one
can take it . 

Comment 29

[Aaron Leventhal]

Bernie, do you want to take a look at this one?

Comment 30

[Bernie McGuire]

Aaron, Yes I'll look at this, it looks interesting. (If you need this fixed in 
the next couple of days, someone else might be a better choice. I can look at 
it over the next week or so, if that's timely enough)

Bernie

Comment 31

[:deanis74]

+  PRBool bSnapDefaultButton;
+  if(SystemParametersInfo(95/*SPI_GETSNAPTODEFBUTTON*/, 0, &bSnapDefaultButton,
0)){

When you're looking at it, I would do this instead:

PRBool bSnapDefaultButton;
SystemParametersInfo(SPI_GETSNAPTODEFBUTTON, 0, &bSnapDefaultButton, 0);
if (bSnapDefaultButton) {

SystemParametersInfo returns non-zero if it succeeds, you need to check the
pvParam value.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Jo Hermans]

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

Comment 34

[Michel Plungjan]

Still unsolved in 1.4b on XP
build 2003050714

Comment 35

[:deanis74]

Still unsolved, really??  Wow, is that why this bug is still open and not marked
as fixed?  Well whaddya know!

Comment 36

[Michel Plungjan]

Just trying to jog... I apologise if that is out of order...

Comment 37

[Dave Townsend [:mossop]]

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

Comment 38

[Jo Hermans]

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

Comment 39

[Phil Ringnalda (:philor)]

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

Comment 43

[Bill Merrow]

This is a serious issue with uptake of Firefox - all (repeating - ALL!) Windows programs now properly handle this behavior.  Firefox 3 is the only thing I have seen in years that fails.

Comment 45

[Arthur]

Reassigning to default as Gilbert doesn't seem to be working on this anymore.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

This can fix this bug in almost cases.

However, if the window is <window> element case, this patch doesn't work fine. And also if <wizard> has a default button which is not "next" and "finish" button, it's not work fine.

The <window> issue, I have no idea for fix it. The later, I used very adhoc way.

I need more time before first review, because some code will changed by big patch (e.g., nsIWidget), so, I should wait the landing.

If you have good idea, please tell me, thanks.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> data:application/vnd.mozilla.xul+xml,<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><button>button1</button><button default="true">button2</button></window>

Oh, we don't support default button on <window>. The button2 is displayed as default button, but enter keypress event doesn't processed as button2 click. So, currently, XUL applications need to process default button behavior themselves on <window>. E.g., On <dialog> and <wizard>, the default button behavior is processed by them. So, it seems that the XUL applications should snap mouse cursor to default button themselves with the new API of my latest patch.

Comment 48

[Neil Deakin]

(In reply to comment #47)

> Oh, we don't support default button on <window>. The button2 is displayed as
> default button, but enter keypress event doesn't processed as button2 click.
> So, currently, XUL applications need to process default button behavior
> themselves on <window>.

<window> doesn't and shouldn't have default buttons, so this feature should have no effect on them.

This code doesn't belong in the command dispatcher. I'd suggest nsIDOMChromeWindow instead.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Thank you, Deakin.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.2

UL code can only call nsIDOMChromeWindow::snapMouseCursorToDefaultButton. That checks whether it should snap the mouse cursor to the center of given default button with nsIWidget::ShouldCursorSnapToDefauldButton. (So, maybe, nsIDOMChromeWindow::snapMouseCursorToDefaultButton misleads. Sounds like it *always* snaps. do you have better idea?) nsIWidget::ShouldCursorSnapToDefauldButton is only implemented on Windows (I'm not sure whether other platforms have similar feature). It checks the system setting by registry. And also it checks the widget is focused, because if the window is opened in background, it should not move the mouse cursor. If nsIWidget::ShouldCursorSnapToDefauldButton returns TRUE, nsIDOMChromeWindow::snapMouseCursorToDefaultButton calls nsIWidget::MoveMouseCursorTo with the center position of the button in screen coordinates.

So, this feature is dangerous if non-chrome application can use. Therefore, nsIDOMChromeWindow::snapMouseCursorToDefaultButton checks UniversalXPConnect capability.

The automate test is simple. It cuts all mouse event first. nsIDOMChromeWindow::snapMouseCursorToDefaultButton fires dummy mouse move event if it moves the mouse cursor. However, the test uses setTimeout for checking that nsIDOMChromeWindow::snapMouseCursorToDefaultButton doesn't move the mouse cursor. This could cause the random test failure.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.2.1

updating for latest trunk.

Comment 54

[Neil Deakin]

Comment on attachment 374234 [details] [diff] [review]
Patch v3.2.1

>+  // This method should not work on any XUL elements of web contents.  Because
>+  // this method can be used to control the mouse cursor.
>+  PRBool hasCap = PR_FALSE;
>+  if (NS_FAILED(nsContentUtils::GetSecurityManager()->
>+        IsCapabilityEnabled("UniversalXPConnect", &hasCap)) || !hasCap) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }

nsGlobalChromeWindow only applies to chrome windows, so I don't think this is necessary.


>+
>+  PRBool isTesting =
>+           nsContentUtils::GetBoolPref("ui.snap_cursor_testing", PR_FALSE);

Not clear what this is for.


>+  // Don't snap to a button when this is called from a sub document.
>+  nsCOMPtr<nsIDocument> doc =
>+    do_QueryInterface(nsContentUtils::GetDocumentFromContext());
>+  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
>+  if (doc->GetParentDocument())
>+    return NS_OK;

GlobalChromeWindows should never have a parent.


>+  // Get a destination point of mouse cursor which is center of the button.

Reword: 'get the center of the button and position the mouse there'


>+  nsPresShellIterator iter(content->GetDocument());
>+  nsCOMPtr<nsIPresShell> shell;
>+  while ((shell = iter.GetNextShell()) && !frame) {
>+    frame = shell->GetPrimaryFrameFor(content);
>+  }

GetPrimaryShell should be sufficient here. The other presshells are only used for printing.


>+  if (isTesting) {
>+    // Synthesize mouse move event for testing.
>+    nsIDocShell* docShell = GetDocShell();
>+    NS_ENSURE_TRUE(docShell, NS_ERROR_UNEXPECTED);
>+    nsCOMPtr<nsIPresShell> winShell;
>+    docShell->GetPresShell(getter_AddRefs(winShell));
>+    NS_ENSURE_TRUE(winShell, NS_ERROR_UNEXPECTED);
>+    nsIFrame* rootFrame = winShell->GetRootFrame();
>+    NS_ENSURE_TRUE(rootFrame, NS_ERROR_UNEXPECTED);
>+    nsPoint nearestWidgetOffset;
>+    nsCOMPtr<nsIWidget> nearestWidget =
>+      rootFrame->GetView()->GetNearestWidget(&nearestWidgetOffset);
>+    nsIntRect nearestWidgetRect;
>+    rv = nearestWidget->GetScreenBounds(nearestWidgetRect);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIDOMWindowUtils> utils;
>+    rv = GetInterface(NS_GET_IID(nsIDOMWindowUtils), getter_AddRefs(utils));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ENSURE_TRUE(utils, NS_ERROR_UNEXPECTED);
>+    rv = utils->SendMouseEvent(NS_LITERAL_STRING("mousemove"),
>+           float(pt.x - nearestWidgetRect.x - nearestWidgetOffset.x),
>+           float(pt.y - nearestWidgetRect.y - nearestWidgetOffset.y),
>+           0, 0, 0, PR_FALSE);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

What is this supposed to be testing? It doesn't seem to belong here.

Regardless, you should be able to just use getBoundingClientRect and append pt.x and pt.y to left and top to get the mousemove coordinate.


>diff --git a/toolkit/content/tests/chrome/Makefile.in b/toolkit/content/tests/chrome/Makefile.in
> 		window_keys.xul \
>+		test_cursorsnap.xul \
>+		window_cursorsnap_dialog.xul \
>+		window_cursorsnap_wizard.xul \

You didn't include these new tests in the patch.


>+    /**
>+     * Whether we should snap the mouse cursor to default button of a dialog.
>+     * aResult is true when we should do. Otherwise, false.
>+     */
>+    NS_IMETHOD ShouldCursorSnapToDefauldButton(PRBool* aResult) = 0;

'Default' not 'Defauld'

I'm not sure it's worth having a separate widget method to check this. Can it not just be incorporated into MoveMouseCursorTo?


>+NS_IMETHODIMP
>+nsWindow::ShouldCursorSnapToDefauldButton(PRBool* aResult)
>+{
>+  NS_PRECONDITION(aResult, "aEnabled is null");

'aResult is null'

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

First, thank you for your review.

(In reply to comment #54)
>+
>+  PRBool isTesting =
>+           nsContentUtils::GetBoolPref("ui.snap_cursor_testing", PR_FALSE);

Not clear what this is for

Sorry, I should document it by comment. For automated testing, we need two additional behaviors for this.
1. We need to ignore the system settings and the window condition (this feature should work only when the created window is activated).
2. We need to generate a synthesized mouse event, see below.

This pref is set to true only from the automated tests.

> >+  if (isTesting) {
> >+    // Synthesize mouse move event for testing.
> >+    nsIDocShell* docShell = GetDocShell();
> >+    NS_ENSURE_TRUE(docShell, NS_ERROR_UNEXPECTED);
> >+    nsCOMPtr<nsIPresShell> winShell;
> >+    docShell->GetPresShell(getter_AddRefs(winShell));
> >+    NS_ENSURE_TRUE(winShell, NS_ERROR_UNEXPECTED);
> >+    nsIFrame* rootFrame = winShell->GetRootFrame();
> >+    NS_ENSURE_TRUE(rootFrame, NS_ERROR_UNEXPECTED);
> >+    nsPoint nearestWidgetOffset;
> >+    nsCOMPtr<nsIWidget> nearestWidget =
> >+      rootFrame->GetView()->GetNearestWidget(&nearestWidgetOffset);
> >+    nsIntRect nearestWidgetRect;
> >+    rv = nearestWidget->GetScreenBounds(nearestWidgetRect);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+    nsCOMPtr<nsIDOMWindowUtils> utils;
> >+    rv = GetInterface(NS_GET_IID(nsIDOMWindowUtils), getter_AddRefs(utils));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+    NS_ENSURE_TRUE(utils, NS_ERROR_UNEXPECTED);
> >+    rv = utils->SendMouseEvent(NS_LITERAL_STRING("mousemove"),
> >+           float(pt.x - nearestWidgetRect.x - nearestWidgetOffset.x),
> >+           float(pt.y - nearestWidgetRect.y - nearestWidgetOffset.y),
> >+           0, 0, 0, PR_FALSE);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  }
> 
> What is this supposed to be testing? It doesn't seem to belong here.
> 
> Regardless, you should be able to just use getBoundingClientRect and append
> pt.x and pt.y to left and top to get the mousemove coordinate.

I think that we can only test this feature by a mouse move event on the button. However, mouse move events are not usable for testing. Because real mouse events can cause unexpected failures. So, we need to suppress the unexpected mouse events which come from the real mouse of the system.

I added nsIDOMWindowUtils::disableNonTestMouseEvents for testing in bug 442774. The missing test cases call this method before testing. Then, the tests only dispatch synthesized mouse events. This code generates the event.

> >diff --git a/toolkit/content/tests/chrome/Makefile.in b/toolkit/content/tests/chrome/Makefile.in
> > 		window_keys.xul \
> >+		test_cursorsnap.xul \
> >+		window_cursorsnap_dialog.xul \
> >+		window_cursorsnap_wizard.xul \
> 
> You didn't include these new tests in the patch.

I'm sorry. Unfortunately, the new files are only in my house, I don't have them in my laptop computer :-(
# I'll post the new patch 5/2 or later.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0

This moves the main logic to nsWindow. And renames the new methods of nsIDOMChromeWindow and nsIWidget. By these changes, ShouldCursorSnapToDefauldButton and MoveMouseCursorTo are merged. These changes may be better for XP level developers.

Comment 57

[Neil Deakin]

> I think that we can only test this feature by a mouse move event on the button.

I don't think we should be adding extra code just for the benefit of a test. Unless I missing the purpose here, you can replace all of the test-specific code with one line of script (synthesizeMouse) in the test itself.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #57)
> Unless I missing the purpose here, you can replace all of the test-specific
> code with one line of script (synthesizeMouse) in the test itself.

Excuse me, I'm not sure what you said. I want to test whether nsWindow::OnDefaultButtonLoaded sets the mouse cursor to over the default button. In other words, I want to test the new APIs, and also I want to test whether they are called correctly. If we synthesize a mouse event from the tests, what is tested?

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil Deakin, would you check this?

I don't understand your comment 57 well. Probably you don't like the testing code is in nsWindow's actual code. But I don't have better idea for testing the feature.

Comment 60

[Neil Deakin]

(In reply to comment #58)
> (In reply to comment #57)
> > Unless I missing the purpose here, you can replace all of the test-specific
> > code with one line of script (synthesizeMouse) in the test itself.
> 
> Excuse me, I'm not sure what you said. I want to test whether
> nsWindow::OnDefaultButtonLoaded sets the mouse cursor to over the default
> button. In other words, I want to test the new APIs, and also I want to test
> whether they are called correctly. If we synthesize a mouse event from the
> tests, what is tested?

Does Windows send a mousemoved event when SetCursorPos is called?

If yes, shouldn't that make it's way to button anyway?
If no, should we instead be sending a fake mouse event even when we aren't testing?

For example, calling this method for a button with a :hover style should move the mouse cursor and enable the style.

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #60)
> Does Windows send a mousemoved event when SetCursorPos is called?
> 
> If yes, shouldn't that make it's way to button anyway?

A WM_MOUSEMOVE message is always sent after SetCursorPos API even if the mouse cursor is not moved by it. However, we need to dispatch a "synthesized" mouse event for the testing. Because the tests stop to handle the "normal" mouse move events at start. (Because if the tests don't stop to handle them, the real mouse events cause some random test failures.)

Comment 62

[Neil Deakin]

(In reply to comment #61)
> Because the tests stop to handle the "normal" mouse move
> events at start.

Can that be disabled temporarily for this test? I'd think that if Windows could send a real event it would be a better test anyway.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #62)
> (In reply to comment #61)
> > Because the tests stop to handle the "normal" mouse move
> > events at start.
> 
> Can that be disabled temporarily for this test? I'd think that if Windows could
> send a real event it would be a better test anyway.

If the tests doesn't stop to handle the real mouse events, the unsnapped test can fail by a real mouse event. At the testing, any mouse move events should not come to the hidden/disabled button. But disabled button can be fired some mouse move events if it's handling the real mouse events.

And also, if some features of this patch are broken by another patch, the test should detect it. But by a real mouse event, it could fail to detect the failure.

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.0

This patch always sends the mouse move event after SetCursorPos. nsWindow of Win32 doesn't dispatch a mouse move event if the mouse cursor isn't changed the position actually.

# I'm not sure why such behavior. I guess that some mouse events (mouse move, mouse enter, mouse exit) are synthesized for hover state at window opening. But maybe they don't have native events for plug-ins.

So, the event might be verbose, but this approach doesn't generate a difference between testing and actual behavior.

And also I improves the automated tests. If they failed the some tests by unexpected timeout, they retry the failed tests with longer timeout value.

Comment 65

[Neil Deakin]

(In reply to comment #63)
> If the tests doesn't stop to handle the real mouse events, the unsnapped test
> can fail by a real mouse event.

There are a number of existing tests that would fail if the real mouse was moved (ones that test tooltips for example), yet don't disable the events. I don't think it is something worth worrying about.

The fundamental problem I have with this patch is that different behaviour is occurring in test mode versus not test mode, and I'd rather test the real behaviour than be concerned about real mouse events that don't matter in practice.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.1

fix bustage on WinCE.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v6.0

Oh, I missed to catch your comment at posting Patch v5.1, sorry.

O.K., this patch doesn't disable the normal mouse events. So, this patch tests the normal behaviors (excepting checking the system settings).

Therefore, this automated test fails by following conditions even if the actual behaviors are expected:

1. The mouse cursor is moved by users.
2. The parent window is deactive at opening the dialog/wizard.

Note that there is a hack of the end of nsWindow::OnDefaultButtonLoaded. The mouse move event and related events are not synthesized at opening a window. Therefore, we need to dispatch an NS_MOUSE_MOVE event by next WM_MOUSEMOVE message. By this reason, we need to ensure that the gLastMouseMovePoint is not same as the position of SetCursorPos API.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v6.0.1

updating for latest trunk.

Comment 69

[Neil Deakin]

Comment on attachment 386219 [details] [diff] [review]
Patch v6.0.1

Seems OK. Does it affect key events firing while scrolling?

You might want to ask Chris Thomas, Mano or the other Neil to review instead as they might have more insight into if this would break something. It is the right fix to do though.

Comment 70

[Neil Deakin]

Whoops, wrong bug :(

Comment 71

[Neil Deakin]

Comment on attachment 386219 [details] [diff] [review]
Patch v6.0.1

These are the real comments:

>+nsIWidget*
>+nsGlobalWindow::GetNearetWidget()
>+{

This should be 'GetNearestWidget'

>+  nsIDocShell* docShell = GetDocShell();
>+  NS_ENSURE_TRUE(docShell, nsnull);
>+  nsCOMPtr<nsIPresShell> winShell;

Call this presShell not winShell.

>+  // Get the button rect in screen coordinates.
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(aDefaultButton));
>+  NS_ENSURE_TRUE(content, NS_ERROR_FAILURE);
>+  nsIDocument *doc = content->GetDocument();

use GetCurrentDoc() instead of GetDocument()

>+  rv = widget->OnDefaultButtonLoaded(buttonRect);
>+  if (rv == NS_ERROR_NOT_IMPLEMENTED)
>+    return NS_OK;
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;

The last two lines can just be 'return rv'

>+if (navigator.platform.indexOf("Win") != 0) {
>+  // We don't support this feature on the platform.
>+  SimpleTest.finish();
>+} else {

I think it would be better to disable this in the makefile to avoid having to load an unused test.

>+      <property name="defaultButton">

Add readonly="true" here.

>+    /*
>+     * When a default button loaded on dialog or something, this method is
>+     * called.  aButtonRect coordinates are relative to this widget.
>+     */ 
>+    NS_IMETHOD OnDefaultButtonLoaded(const nsIntRect &aButtonRect) = 0;

Change the comment to be:

'Call this method when a dialog is opened which has a default button. The button's
rectangle should be supplied in aButtonRect.'

>+  // The button can be outside of the widget.
>+  // E.g., it could be hidden by scrolling.
>+  if (!widgetRect.Intersects(buttonRect) ||
>+      !widgetRect.Contains(centerOfButton)) {
>+    return NS_OK;
>+  }

Is there a reason to check both of these?

>+
>+  if (!::SetCursorPos(centerOfButton.x, centerOfButton.y)) {
>+    NS_ERROR("SetCursorPos failed");
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // Hack: if the mouse cursor is pointed at gLastMouseMovePoint, the next
>+  // mouse move event which is generated by SetCursorPos event will not be
>+  // dispached.  Therefore, we ensure that gLastMouseMovePoint is moved to
>+  // another point here.
>+  sLastMouseMovePoint.x = centerOfButton.x + 1;

The comment needs to be updated to reflect the change to sLastMouseMovePoint.


The tests seem overly complicated. In which cases do you need to keep retrying the test?

Can the 1000ms timeout be reduced? It seems a shame to have this test require 6 seconds to run.

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #71)
> The tests seem overly complicated. In which cases do you need to keep retrying
> the test?

If the expected events aren't fired during the waiting time, the failure might be caused by the system slowdown. So, these tests can be random failure tests. For reducing the random failure, they need to retry if the tests failed by the timeout. I experienced similar issue. See bug 491712 and bug 485994.

> Can the 1000ms timeout be reduced?

OK. It's possible.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v7.0

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 386771 [details] [diff] [review]
Patch v7.0

Thank you, Enn.

Ere, would you review the Win32 part?

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ere?

Comment 76

[Ere Maijala (slow)]

Comment on attachment 386771 [details] [diff] [review]
Patch v7.0

r+ with these nits fixed:
   gfxASurface             *GetThebesSurface();
+  NS_IMETHOD OnDefaultButtonLoaded(const nsIntRect &aButtonRect);

Fix indentation.

+ * Called this after the dialog is loaded and it has a default

Remove "this".

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v7.0.1

Thank you, Ere.

Roc, this patch needs to get the nearest widget in nsGlobalWindow. So, this may conflict your child widget removing patch that is very big. If you'll land it soon, I wait to land this.

Comment 78

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

+  // Don't snap when we are not active.
+  if (::GetActiveWindow() != ::GetForegroundWindow())
+    return NS_OK;

Shouldn't we also check that GetActiveWindow is an ancestor of mWnd? This app might have another window that happens to be active.

Why do we need a Gecko pref for this?

+  nsIntRect widgetRect;
+  nsresult rv = GetScreenBounds(widgetRect);
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsIntRect buttonRect(aButtonRect + widgetRect.TopLeft());
+
+  nsIntPoint centerOfButton(buttonRect.x + buttonRect.width / 2,
+                            buttonRect.y + buttonRect.height / 2);
+
+  // The center of the button can be outside of the widget.
+  // E.g., it could be hidden by scrolling.
+  if (!widgetRect.Contains(centerOfButton)) {
+    return NS_OK;
+  }

Shouldn't this be done in NotifyDefaultButtonLoaded?

+  // Hack: if the mouse cursor is pointed at sLastMouseMovePoint, the next
+  // mouse move event which is generated by SetCursorPos API will not be
+  // dispached.  Therefore, we ensure that sLastMouseMovePoint is moved to
+  // another point here.
+  sLastMouseMovePoint.x = centerOfButton.x + 1;

So if the user moves the mouse one pixel to the right, that will not be noticed?

GetNearestWindow should continue to work after the compositor stuff lands, so that's not a problem.

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v7.1

(In reply to comment #78)
> +  // Don't snap when we are not active.
> +  if (::GetActiveWindow() != ::GetForegroundWindow())
> +    return NS_OK;
> 
> Shouldn't we also check that GetActiveWindow is an ancestor of mWnd? This app
> might have another window that happens to be active.

Oh, right.

> Why do we need a Gecko pref for this?

for automated testing. the setting is off in the default system settings. so, we cannot test it without the pref.

> +  nsIntRect widgetRect;
> +  nsresult rv = GetScreenBounds(widgetRect);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsIntRect buttonRect(aButtonRect + widgetRect.TopLeft());
> +
> +  nsIntPoint centerOfButton(buttonRect.x + buttonRect.width / 2,
> +                            buttonRect.y + buttonRect.height / 2);
> +
> +  // The center of the button can be outside of the widget.
> +  // E.g., it could be hidden by scrolling.
> +  if (!widgetRect.Contains(centerOfButton)) {
> +    return NS_OK;
> +  }
> 
> Shouldn't this be done in NotifyDefaultButtonLoaded?

Um... I think that it's not good for semantics, but non-Windows platforms don't support this feature, so, it's ok for now.

> +  // Hack: if the mouse cursor is pointed at sLastMouseMovePoint, the next
> +  // mouse move event which is generated by SetCursorPos API will not be
> +  // dispached.  Therefore, we ensure that sLastMouseMovePoint is moved to
> +  // another point here.
> +  sLastMouseMovePoint.x = centerOfButton.x + 1;
> 
> So if the user moves the mouse one pixel to the right, that will not be
> noticed?

ok, I added to DispatchMouseEvent here. By this, sLastMouseMovePoint will be updated in DispatchMouseEvent. Even if there are pending mouse events, the WM_MOUSEMOVE message which is posted by SetCursorPos API will win to another events. So, the hack is needed only when the cursor is already on the center of the default button and there are no pending mouse events.

> GetNearestWindow should continue to work after the compositor stuff lands, so
> that's not a problem.

ok, thanks.

Comment 80

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

> > +  nsIntRect widgetRect;
> > +  nsresult rv = GetScreenBounds(widgetRect);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  nsIntRect buttonRect(aButtonRect + widgetRect.TopLeft());
> > +
> > +  nsIntPoint centerOfButton(buttonRect.x + buttonRect.width / 2,
> > +                            buttonRect.y + buttonRect.height / 2);
> > +
> > +  // The center of the button can be outside of the widget.
> > +  // E.g., it could be hidden by scrolling.
> > +  if (!widgetRect.Contains(centerOfButton)) {
> > +    return NS_OK;
> > +  }
> > 
> > Shouldn't this be done in NotifyDefaultButtonLoaded?
> 
> Um... I think that it's not good for semantics, but non-Windows platforms
> don't support this feature, so, it's ok for now.

What do you mean?

I think it would actually simplify NotifyDefaultButtonLoaded and work better. Currently you compute the button rect in screen coordinates, then make it relative to the widget top-left, and here you're converting it back to screen coordinates. Maybe if you just make the parameter to OnDefaultButtonLoaded be in screen coordinates?

Comment 81

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #80)
> > > +  nsIntRect widgetRect;
> > > +  nsresult rv = GetScreenBounds(widgetRect);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +  nsIntRect buttonRect(aButtonRect + widgetRect.TopLeft());
> > > +
> > > +  nsIntPoint centerOfButton(buttonRect.x + buttonRect.width / 2,
> > > +                            buttonRect.y + buttonRect.height / 2);
> > > +
> > > +  // The center of the button can be outside of the widget.
> > > +  // E.g., it could be hidden by scrolling.
> > > +  if (!widgetRect.Contains(centerOfButton)) {
> > > +    return NS_OK;
> > > +  }
> > > 
> > > Shouldn't this be done in NotifyDefaultButtonLoaded?
> > 
> > Um... I think that it's not good for semantics, but non-Windows platforms
> > don't support this feature, so, it's ok for now.
> 
> What do you mean?
> 
> I think it would actually simplify NotifyDefaultButtonLoaded and work better.
> Currently you compute the button rect in screen coordinates, then make it
> relative to the widget top-left, and here you're converting it back to screen
> coordinates. Maybe if you just make the parameter to OnDefaultButtonLoaded be
> in screen coordinates?

I designed nsIWidget::OnDefaultButtonLoaded is just a notification of the default button loaded on the widget. So, it's NOT a command that moves the cursor to the center of the button. And also shouldn't the coordinates of nsIWidget be the relative from the widget? Isn't the change a cause of the confusing of the nsIWidget users?

Comment 82

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

OK, that code can stay I guess. But I still don't understand this stuff?

+  // Hack: if the mouse cursor is pointed at sLastMouseMovePoint, the next
+  // mouse move event which is generated by SetCursorPos API will not be
+  // dispached.  Therefore, we ensure that sLastMouseMovePoint is moved to
+  // another point here. And also, we shouldn't kill the actual mouse move
+  // event by this hack, so, we need to dispatch the mouse move event for the
+  // SetCursorPos here.
+  // NOTE: Even if there are other mouse events, it's ok because SetCursorPos
+  // posts a WM_MOUSEMOVE, so, the message will be processed after the pending
+  // input events.  This hack is needed only when the mouse cursor is already
+  // on the point and there are no pending WM_MOUSEMOVE messages.
+  sLastMouseMovePoint.x = centerOfButton.x + 1;
+  centerOfButton -= widgetRect.TopLeft();
+  DispatchMouseEvent(NS_MOUSE_MOVE, 0,
+                     MAKELPARAM(centerOfButton.x, centerOfButton.y));

if sLastMouseMovePoint is already (centerOfButton.x, centerOfButton.y), why do we need to ensure that an event is dispatched? It doesn't matter whether we set the position or we dispatch an event, the mouse is already in the right place.

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #82)
> if sLastMouseMovePoint is already (centerOfButton.x, centerOfButton.y), why do
> we need to ensure that an event is dispatched? It doesn't matter whether we set
> the position or we dispatch an event, the mouse is already in the right place.

Because if there is already the mouse cursor at the point, the default button doesn't have the :hover state. It might be another bug and not important.

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #83)
> (In reply to comment #82)
> > if sLastMouseMovePoint is already (centerOfButton.x, centerOfButton.y), why do
> > we need to ensure that an event is dispatched? It doesn't matter whether we set
> > the position or we dispatch an event, the mouse is already in the right place.
> 
> Because if there is already the mouse cursor at the point, the default button
> doesn't have the :hover state. It might be another bug and not important.

Ah, it's important for the automated tests. If I remove the code, the tests will be failed. Because the tests checking the mouse move events which are fired by SetCursorPos.

Comment 85

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

(In reply to comment #83)
> (In reply to comment #82)
> > if sLastMouseMovePoint is already (centerOfButton.x, centerOfButton.y), why do
> > we need to ensure that an event is dispatched? It doesn't matter whether we
> > set the position or we dispatch an event, the mouse is already in the right
> > place.
> 
> Because if there is already the mouse cursor at the point, the default button
> doesn't have the :hover state.

It should. Reflow generates a synthetic mouse event that should trigger the :hover state.

Can't you make the tests work by moving the window (or the button) each time before you trigger OnDefaultButtonLoaded? That should ensure the mouse has to move and a mouse event fires.

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v8.0

ok, this can pass the all new tests.

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

thank you, roc.

I'll land it tonight (JST).

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/08c3952a49b3

landed.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The new tests have random failure issue, see bug 504313.

Comment 90

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Masayuki, it doesn't work for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090718 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090718050152

While all other applications snap the mouse to the default button Minefield still doesn't do that. The mouse remains on its last position. I have checked with several dialogs and wizards but without success.

Why we have a "no" in the summary. Everything here including the patch makes clear we want to have it implemented. Removing this word and reopening bug.

Comment 91

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #90)
> Masayuki, it doesn't work for me with Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9.2a1pre) Gecko/20090718 Minefield/3.6a1pre (.NET CLR 3.5.30729)
> ID:20090718050152
> 
> While all other applications snap the mouse to the default button Minefield
> still doesn't do that. The mouse remains on its last position. I have checked
> with several dialogs and wizards but without success.

I guess that your mouse utils snaps the mouse cursor itself and it doesn't set it to the registry. Please check the value of your registry. The key is:
SnapToDefaultButton of \HKEY_CURRENT_USER\Control Panel\Mouse.
If the value is 0, it's disabled, otherwise, it's enabled.

Comment 92

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It's set to 1 so it is enabled. As said it works fine in other applications. So something is missing in our code base.

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Um, it's strange. Can you reproduce it on clean profile? And also can you reproduce it after you create |ui.cursor_snapping.always_enabled| and set to true?

Comment 94

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Er, ok. I can reproduce it only on WinXP. I'll check it.

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #94)
> Er, ok. I can reproduce it only on WinXP. I'll check it.

Ugh, this may be wrong. SetCursorPos API doesn't work fine on VMware (VMware Tools may cause it). Henrik, do you test on a real machine? And would you check whether the default button style is changed to hovered style or not? On VMware, the real cursor isn't moved to default button, but the style is changed to hovered style (blue border button to orange border button).

Comment 96

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And can other people reproduce the problem? If the problem depends on the environment, we should not reopen this bug and should file a new bug with the detail of the system settings and the list of the installed utilities.

Comment 97

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I confirmed that the current trunk build works for me on WinXP and Win2k too.

Comment 98

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ok, I remarked this to RESOLVED FIXED because I and another tester couldn't reproduce the Henrik's problem. So, I believe that the landed patch works fine in most systems. But when it's with someone, it may not work fine. Henrik, please file a new bug and block this bug by it. And you have to report your environment (e.g., the model number of the PC, the model of the mouse and its driver version and the installed utilities which is running always), I cannot fix your problem if I cannot create such environment.

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

oops, sorry for the spam.

-> FIXED

Comment 100

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #95)
> Ugh, this may be wrong. SetCursorPos API doesn't work fine on VMware (VMware
> Tools may cause it). Henrik, do you test on a real machine? And would you check
> whether the default button style is changed to hovered style or not? On VMware,
> the real cursor isn't moved to default button, but the style is changed to
> hovered style (blue border button to orange border button).

Right. Good assumption. I'm running VMware cause I don't have a real Windows machine. After removing the VMware Tools snapping works fine. I'll file a new bug for the remaining issue.

Marking as verified fixed with builds on Windows XP and Windows 7 and Vmware Tools disabled.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090719 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090719043021

Comment 101

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I write the document for XUL application developers.
https://developer.mozilla.org/en/NsIDOMChromeWindow#notifyDefaultButtonLoaded%28%29