Closed Bug 76053 Opened 23 years ago Closed 15 years ago

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

Categories

(Core :: XUL, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: gekacheka, Assigned: masayuki)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: access, arch, dev-doc-complete)

Attachments

(1 file, 15 obsolete files)

26.48 KB, patch
Details | Diff | Splinter Review
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).
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.
yeah...
Assignee: asa → danm
Component: Browser-General → XP Toolkit/Widgets
Keywords: arch, helpwanted
QA Contact: doronr → jrgm
->future
Summary: Windows integration: no "Snap to default button in dialog boxes" → Windows integration: no "Snap to default button in dialog boxes"
Target Milestone: --- → Future
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated summary to include search term "mouse".
Summary: Windows integration: no "Snap to default button in dialog boxes" → Windows mouse integration: no "Snap to default button in dialog boxes"
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.


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.
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?
Keywords: access
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.
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.
Assignee: danm → aaronl
Target Milestone: Future → ---
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.
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.
Does anyone know how we find out if the snap-to default button setting is set in
Windows? Then we could do this ourselves.
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/
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?


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.
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)
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.
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.
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.
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?
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.
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.
Attached patch Patch Prototype, Need Help (obsolete) — Splinter Review
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?
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.


On Windows you use SetCursorPos().

BOOL SetCursorPos(
    int X,	// horizontal position  
    int Y 	// vertical position
   );
I won't get to this. Jim, do you still want it? 
Assignee: aaronl → jim.song
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 . 
Assignee: jim.song → gilbert.fang
Bernie, do you want to take a look at this one?
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
+  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.
*** Bug 166280 has been marked as a duplicate of this bug. ***
*** Bug 201612 has been marked as a duplicate of this bug. ***
Still unsolved in 1.4b on XP
build 2003050714
Still unsolved, really??  Wow, is that why this bug is still open and not marked
as fixed?  Well whaddya know!
Just trying to jog... I apologise if that is out of order...
Attachment #73643 - Attachment is patch: true
*** Bug 311336 has been marked as a duplicate of this bug. ***
*** Bug 264505 has been marked as a duplicate of this bug. ***
*** Bug 360179 has been marked as a duplicate of this bug. ***
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.
Reassigning to default as Gilbert doesn't seem to be working on this anymore.
Assignee: gilbert.fang → jag
Severity: minor → normal
QA Contact: jrgmorrison → xptoolkit.widgets
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Assignee: jag → masayuki
Attachment #73643 - Attachment is obsolete: true
Status: NEW → ASSIGNED
> 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.
(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.
Attached patch Patch v2.0 (obsolete) — Splinter Review
Thank you, Deakin.
Attachment #336532 - Attachment is obsolete: true
Attached patch Patch v3.0 (obsolete) — Splinter Review
Attachment #356475 - Attachment is obsolete: true
Attached patch Patch v3.1 (obsolete) — Splinter Review
Attachment #373856 - Attachment is obsolete: true
Attached patch Patch v3.2 (obsolete) — Splinter Review
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.
Attachment #373873 - Attachment is obsolete: true
Attachment #373988 - Flags: review?(enndeakin)
Attached patch Patch v3.2.1 (obsolete) — Splinter Review
updating for latest trunk.
Attachment #373988 - Attachment is obsolete: true
Attachment #373988 - Flags: review?(enndeakin)
Attachment #374234 - Flags: review?(enndeakin)
Attachment #374234 - Flags: review?(enndeakin) → review-
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'
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.
Attached patch Patch v4.0 (obsolete) — Splinter Review
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.
Attachment #374234 - Attachment is obsolete: true
Attachment #375458 - Flags: review?(enndeakin)
> 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.
(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?
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.
(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.
(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.)
(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.
(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.
Attached patch Patch v5.0 (obsolete) — Splinter Review
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.
Attachment #375458 - Attachment is obsolete: true
Attachment #375458 - Flags: review?(enndeakin)
Attachment #384218 - Flags: review?(enndeakin)
(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.
Attached patch Patch v5.1 (obsolete) — Splinter Review
fix bustage on WinCE.
Attachment #384218 - Attachment is obsolete: true
Attachment #384218 - Flags: review?(enndeakin)
Attachment #384219 - Flags: review?(enndeakin)
Attached patch Patch v6.0 (obsolete) — Splinter Review
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.
Attachment #384219 - Attachment is obsolete: true
Attachment #384219 - Flags: review?(enndeakin)
Attachment #384230 - Flags: review?(enndeakin)
Attached patch Patch v6.0.1 (obsolete) — Splinter Review
updating for latest trunk.
Attachment #384230 - Attachment is obsolete: true
Attachment #384230 - Flags: review?(enndeakin)
Attachment #386219 - Flags: review?(enndeakin)
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.
Attachment #386219 - Flags: review?(enndeakin) → review+
Whoops, wrong bug :(
Attachment #386219 - Flags: review+ → review?(enndeakin)
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.
(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.
Attached patch Patch v7.0 (obsolete) — Splinter Review
Attachment #386219 - Attachment is obsolete: true
Attachment #386219 - Flags: review?(enndeakin)
Attachment #386771 - Flags: review?(enndeakin)
Attachment #386771 - Flags: review?(enndeakin) → review+
Comment on attachment 386771 [details] [diff] [review]
Patch v7.0

Thank you, Enn.

Ere, would you review the Win32 part?
Attachment #386771 - Flags: review?(emaijala)
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".
Attachment #386771 - Flags: review?(emaijala) → review+
Attached patch Patch v7.0.1 (obsolete) — Splinter Review
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.
Attachment #386771 - Attachment is obsolete: true
Attachment #388615 - Flags: superreview?(roc)
+  // 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.
Attached patch Patch v7.1 (obsolete) — Splinter Review
(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.
Attachment #388615 - Attachment is obsolete: true
Attachment #388615 - Flags: superreview?(roc)
Attachment #388623 - Flags: superreview?(roc)
> > +  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?
(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?
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.
(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.
(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.
(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.
Attached patch Patch v8.0Splinter Review
ok, this can pass the all new tests.
Attachment #388623 - Attachment is obsolete: true
Attachment #388623 - Flags: superreview?(roc)
Attachment #388638 - Flags: superreview?(roc)
Attachment #388638 - Flags: superreview?(roc) → superreview+
thank you, roc.

I'll land it tonight (JST).
http://hg.mozilla.org/mozilla-central/rev/08c3952a49b3

landed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
The new tests have random failure issue, see bug 504313.
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.
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
Summary: Windows mouse integration: no "Snap to default button in dialog boxes" → Windows mouse integration: "Snap to default button in dialog boxes"
(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.
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.
Status: REOPENED → ASSIGNED
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?
Er, ok. I can reproduce it only on WinXP. I'll check it.
(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).
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.
I confirmed that the current trunk build works for me on WinXP and Win2k too.
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.
oops, sorry for the spam.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
(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
Status: RESOLVED → VERIFIED
Blocks: 685991
Depends on: 1197497
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: