Open
Bug 550528
Opened 15 years ago
Updated 2 years ago
Click count (event.detail) is different compared to Opera and Chrome when quadruple (or more) clicking
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
REOPENED
People
(Reporter: martijn.martijn, Unassigned)
References
()
Details
(Keywords: testcase)
Attachments
(3 files)
See testcase, when clicking rapidly 4 times, I get as result:
For Opera 10.5:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 4
For Google Chrome 4.0:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 4
For Mozilla trunk:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 2
dblclick with detail (== click count): 2
When clicking rapidly 5 times, I get as result:
For Opera 10.5:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 4
click with detail (== click count): 1
For Google Chrome 4.0:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 4
click with detail (== click count): 5
For Mozilla trunk:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
It seems to me that what Opera is doing, is making the most since. The click count max is there 4, and then it restarts with 1. Google Chrome seems to increase the click count number indefinitely.
I have no idea what logic Mozilla is using, but it doesn't seem to make much sense to me.
I can test Internet Explorer, if wanted (I have to write a different testcase for that, suited for IE's scripting model).
Comment 1•15 years ago
|
||
This is only visible on Windows and Linux. OS X isn't affected. It's not a regression. I can reproduce it even with Shiretoko builds.
OS: Windows 7 → All
Hardware: x86 → All
Comment 2•15 years ago
|
||
What does OSX output on Mozilla?
Comment 3•15 years ago
|
||
On OS X the count doesn't get reset and the value is incremented all the time:
click with detail (== click count): 1
click with detail (== click count): 2
dblclick with detail (== click count): 2
click with detail (== click count): 3
click with detail (== click count): 4
click with detail (== click count): 5
click with detail (== click count): 6
click with detail (== click count): 7
click with detail (== click count): 8
click with detail (== click count): 9
click with detail (== click count): 10
click with detail (== click count): 11
click with detail (== click count): 12
click with detail (== click count): 13
Comment 4•12 years ago
|
||
This bug still presents in Firefox 16.
Is it big thing to fix?
Quad click is used in ACE editor. For now it implements it itself due to this bug, but click interval cannot be read from user OS preferences.
Comment 5•12 years ago
|
||
This bug still presents in Firefox 18.
Is it big thing to fix?
I think Chrome (and IE) behavior is the best, because we can easily "reset" it manually if want after any number. For example:
var max = 3;
var count = ((e.detail + max - 1) % max) + 1;
1 - 1
2 - 2
3 - 3
4 - 1
...
Thanks for attention.
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment hidden (mozreview-request) |
A reference for implementing higher-order clicks: https://blogs.msdn.microsoft.com/oldnewthing/20041018-00/?p=37543 .
Note, however, that, according to my tests, the explanation about `GetSystemMetrics(SM_C...DOUBLECLK)` is wrong. In the Firefox codebase it's already done correctly and not as described in the above blog post. If desired, I can post the test code. If desired, I can also post information of how to create a test C# app that queries `GetSystemMetrics()` and `GetSystemMetricsForDpi()` under different DPI awareness circumstances.
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
See commit message.
Attachment #8989796 -
Flags: review?(overholt)
Attachment #8989796 -
Flags: review?(bugs)
Updated•6 years ago
|
Attachment #8989796 -
Flags: review?(overholt)
Comment 9•6 years ago
|
||
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
this is windows specific. :jimm should perhaps review, or :masayuki.
Masayuki, are you familiar with this code.
Attachment #8989796 -
Flags: review?(bugs) → review?(masayuki)
Comment 10•6 years ago
|
||
Important information in this context
=====================================
MSDN writes:
> Only windows that have the CS_DBLCLKS style can receive WM_LBUTTONDBLCLK messages, which the system generates whenever the user presses, releases, and again presses the left mouse button within the system's double-click time limit. Double-clicking the left mouse button actually generates a sequence of four messages: WM_LBUTTONDOWN, WM_LBUTTONUP, WM_LBUTTONDBLCLK, and WM_LBUTTONUP.
> (https://docs.microsoft.com/en-us/windows/desktop/inputdev/wm-lbuttondblclk)
That means:
- Double-clicks are timed from mouse-down to mouse-down.
- The double-click message is sent on mouse-down.
- The double-click message replaces the mouse-down message.
Comment 11•6 years ago
|
||
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
Felipe, you authored this commit: https://hg.mozilla.org/mozilla-central/rev/f5d0530311c0 . Could you please clarify the changes in the following context? https://reviewboard.mozilla.org/r/254770/diff/1#chunk2.131
Thanks!
Attachment #8989796 -
Flags: review?(felipc)
Comment 12•6 years ago
|
||
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
Felipe, you authored this commit: https://hg.mozilla.org/mozilla-central/rev/f5d0530311c0 . Could you please clarify the changes in the following context? https://reviewboard.mozilla.org/r/254770/diff/1#chunk2.131
Thanks!
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8989796 [details]
> Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
>
> this is windows specific. :jimm should perhaps review, or :masayuki.
>
> Masayuki, are you familiar with this code.
Yeah, I'll do it.
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261748
Still reviewing, but if you will split to some patches as explained in the review comment, I'd be really happy and I could review safer (i.e., looking for unexpected behavior changes etc).
::: commit-message-a9dc5:4
(Diff revision 1)
> +recognition of double-, triple-, quadruple-, quintuple-clicks etc. Use the
> +"testcase" attachment of the bug which is an HTML file with JavaScript.
Please include the URL of attached testcase rather than say '"testcase" attachment of the bug which is an HTML file with Javascript' because when other developers investigate the reason of this change, they want to access any resource pointed by commit message quickly.
::: commit-message-a9dc5:7
(Diff revision 1)
> +
> +Grouping of clicks, based on temporal and spatial boundaries, means the
> +recognition of double-, triple-, quadruple-, quintuple-clicks etc. Use the
> +"testcase" attachment of the bug which is an HTML file with JavaScript.
> +
> +On Mac, Firefox (allegedly) already allows infinite grouping of clicks.
Remove "(allegedly)". I confirmed. And Linux behaves same as Windows. So, you should file a follow up bug for Linux even if we take this patch.
::: commit-message-a9dc5:9
(Diff revision 1)
> +Also, Opera and Chrome allow for infinite grouping of clicks, at least on
> +Windows.
I don't know Opera means Presto or Blink. Anyway, important point is engine, not browser. So, please rewrite this something like:
Blink allows for infinite grouping of clicks on Windows and macOS. Although Blink on Linux and EdgeHTML on Windows do not so.
::: widget/windows/nsWindow.cpp:1024
(Diff revision 1)
> - wc.style = CS_DBLCLKS | aExtraStyle;
> + // Note: The `CS_DBLCLKS` style may not be used because we implement our own
> + // double-, triple- and higher-order click recognition.
> + wc.style = aExtraStyle;
x86 build (i.e., 32bit) binary still support windowed plugin mode. Do you confirm whether this change is safe for plugin window?
::: widget/windows/nsWindow.cpp:1037
(Diff revision 1)
> if (!::RegisterClassW(&wc)) {
> // For older versions of Win32 (i.e., not XP), the registration may
> // fail with aExtraStyle, so we have to re-register without it.
> - wc.style = CS_DBLCLKS;
> + wc.style = 0;
> ::RegisterClassW(&wc);
> }
According to the comment, we could remove this block, but of course, it should be discussed by experts of Win32 programming in another bug.
::: widget/windows/nsWindow.cpp:2762
(Diff revision 1)
> // | | app client chrome | | }
> // | +-----------------------+ | }
> // | | app content | | } area we don't want to invalidate
> // | +-----------------------+ | }
> // | | app client chrome | | }
> - // | +-----------------------+ |
> + // | +-----------------------+ |
Hmm, not related to this bug. Please keep current code or you should file a clean up bug and fix it first if your editor forcibly does this.
::: widget/windows/nsWindow.cpp:4371
(Diff revision 1)
> int16_t aButton, uint16_t aInputSource,
> WinPointerInfo* aPointerInfo)
> {
> - enum
> - {
> - eUnset,
> + /* * * PREPARATION * * */
> +
> + enum { eUnset, ePrecise, eTouch };
if possible, using enum class is better than keeping enum since enum class is really safer for preventing implicit-cast with int and the enum class name explicitly explains what the value means.
::: widget/windows/nsWindow.cpp:4382
(Diff revision 1)
>
> if (!mWidgetListener) {
> return result;
> }
>
> + /* * * GATHER EVENT INFO * * */
Hmm, you do just change some order of the following code which is not directly related to this bug. Please separate such changes to preparation patch. If the patch explain "not changing behavior", reviewers can review easier. So, I'd like you to separate this patch at least 3 patches. One is to clean up some whitespaces and some trivial changes. Second one is to sort out this method without behavior change (or split the method to smaller parts and just call them from this method). Then, finally, you should change the behavior.
Attachment #8989796 -
Flags: review?(masayuki) → review-
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261754
::: widget/windows/nsWindow.cpp:4454
(Diff revision 1)
> + // FIXME @ h-h & reviewers:
> + // `GetSystemMetrics()` is not DPI-aware. Windows 10 introduces
> + // `GetSystemMetricsForDpi()`. Just scaling the return value of the
> + // former function seems to be the wrong way because the following
> + // invocations all return 4*4 px at the moment for the author of this
> + // todo on a single-monitor system with 200 % DPI scaling; the manifest
> + // of the test app contained `<dpiAware>true/PM</dpiAware>`:
> + // GetSystemMetrics (SM_CXDOUBLECLK), GetSystemMetrics (SM_CYDOUBLECLK)
> + // GetSystemMetricsForDpi(SM_CXDOUBLECLK, 96), GetSystemMetricsForDpi(SM_CYDOUBLECLK, 96)
> + // GetSystemMetricsForDpi(SM_CXDOUBLECLK, 192), GetSystemMetricsForDpi(SM_CYDOUBLECLK, 192)
> + // -> How do we support `GetSystemMetricsForDpi()` without compiler
> + // errors? How about using `GetProcAddress()` from `kernel32.dll`?
> + // Is there a predetermined way?
> + // For exemplary code, see:
> + // - https://hg.mozilla.org/mozilla-central/file/tip/xpcom/base/nsSystemInfo.cpp#l420
> + // - https://hg.mozilla.org/mozilla-central/file/tip/widget/windows/WinUtils.cpp#l434
Yes, we use dynamic load when we need to use new API. And yes, you can wrap the API with WinUtils, it's better to refer directly since the callers look simpler.
On the other hand, it's new feature if you do here. So, please separate patch only for supporting DPI. But if it won't fix new regressions caused by your patch, please do it in another bug.
::: widget/windows/nsWindow.cpp:4491
(Diff revision 1)
> + horzMovementThreshold = (short)::GetSystemMetrics(SM_CXDOUBLECLK);
> + vertMovementThreshold = (short)::GetSystemMetrics(SM_CYDOUBLECLK);
FYI: Please use static_cast instead of C-style since easier to search. Anyway, this should be hidden by WinUtils. (If you support high-DPI later, please remove the lines commented out.) Perhaps, making us DPI-aware should be fixed before this bug. Then, this big change becomes safer.
::: widget/windows/nsWindow.cpp:4494
(Diff revision 1)
> + bool insideMovementThreshold =
> + DeprecatedAbs(sLastMousePoint.x - eventPoint.x) < horzMovementThreshold &&
> + DeprecatedAbs(sLastMousePoint.y - eventPoint.y) < vertMovementThreshold;
According to the brief test on Chrome, this may be wrong. Chrome probably compare with first click point instead of last click point. This probably allows users to grow click-count even with moving mouse cursor slowly.
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261772
::: widget/windows/nsWindow.cpp:4407
(Diff revision 1)
> + if (aEventMessage == eMouseExitFromWidget) {
> + event.mExitFrom =
> + IsTopLevelMouseExit(mWnd) ? WidgetMouseEvent::eTopLevel :
> + WidgetMouseEvent::eChild;
> + }
Hmm, making this block spun out from the following switch statement for performance, but I have no idea to keep grouping tasks without this separation...
::: widget/windows/nsWindow.cpp:4431
(Diff revision 1)
> BYTE eventButton;
> switch (aButton) {
> case WidgetMouseEvent::eLeftButton:
> eventButton = VK_LBUTTON;
> break;
> +
> case WidgetMouseEvent::eMiddleButton:
> eventButton = VK_MBUTTON;
> break;
> +
> case WidgetMouseEvent::eRightButton:
> eventButton = VK_RBUTTON;
> break;
> +
> default:
> eventButton = 0;
> break;
> }
Looks like that eventButton is referred only by 2 points and they are not called once. So, perhaps, this should be separated to another method and make them call the new method instead of referring cached result.
::: widget/windows/nsWindow.cpp:4432
(Diff revision 1)
> switch (aButton) {
> case WidgetMouseEvent::eLeftButton:
> eventButton = VK_LBUTTON;
> break;
> +
> case WidgetMouseEvent::eMiddleButton:
> eventButton = VK_MBUTTON;
> break;
> +
> case WidgetMouseEvent::eRightButton:
> eventButton = VK_RBUTTON;
> break;
> +
Usually, we don't insert empty line between |break;| and next |case|. So, keep not inserting empty lines here.
::: widget/windows/nsWindow.cpp:4501
(Diff revision 1)
> switch (aEventMessage) {
> - case eMouseDoubleClick:
> - event.mMessage = eMouseDown;
> - event.button = aButton;
> - sLastClickCount = 2;
> - sLastMouseDownTime = curMsgTime;
> - break;
> case eMouseUp:
> - // remember when this happened for the next mouse down
> + // On next mouse-down, relate to this event data.
> sLastMousePoint.x = eventPoint.x;
> sLastMousePoint.y = eventPoint.y;
> sLastMouseButton = eventButton;
> break;
> +
> case eMouseDown:
> - // now look to see if we want to convert this to a double- or triple-click
> - if (((curMsgTime - sLastMouseDownTime) < (LONG)::GetDoubleClickTime()) &&
> + // Associate click with previously grouped ones? (Forming double-, triple-
> + // or higher-order click depending on temporal and spatial boundaries.)
> + if (curMsgTime - sLastMouseDownTime < (LONG)::GetDoubleClickTime() &&
> insideMovementThreshold &&
> eventButton == sLastMouseButton) {
> - sLastClickCount ++;
> + sLastClickCount++;
> + if (sLastClickCount == 2) {
> + aEventMessage = eMouseDoubleClick;
> + // Note: `event.mMessage` must remain `eMouseDown`!
> + }
> } else {
> - // reset the click count, to count *this* click
> + // Reset - boundaries were not met.
> sLastClickCount = 1;
> }
> - // Set last Click time on MouseDown only
> +
> + // Save last click timestamp only on mouse-down.
> sLastMouseDownTime = curMsgTime;
> break;
> +
> case eMouseMove:
> if (!insideMovementThreshold) {
> + // Reset - mouse has left spatial boundaries.
> sLastClickCount = 0;
> }
> break;
> - case eMouseExitFromWidget:
> +
> - event.mExitFrom =
> - IsTopLevelMouseExit(mWnd) ? WidgetMouseEvent::eTopLevel :
> - WidgetMouseEvent::eChild;
> - break;
> default:
> break;
> }
And also, this |switch| don't need empty lines before each |case|.
::: widget/windows/nsWindow.cpp:4503
(Diff revision 1)
> - // remember when this happened for the next mouse down
> + // On next mouse-down, relate to this event data.
> sLastMousePoint.x = eventPoint.x;
> sLastMousePoint.y = eventPoint.y;
> sLastMouseButton = eventButton;
> break;
So, perhaps, this should be modified only when sLastClickCount is 1.
::: widget/windows/nsWindow.cpp:4517
(Diff revision 1)
> + aEventMessage = eMouseDoubleClick;
> + // Note: `event.mMessage` must remain `eMouseDown`!
Ah, I understand now why eMouseDoubleClick message is used in widget...
::: widget/windows/nsWindow.cpp:4617
(Diff revision 1)
> switch (aEventMessage) {
> case eMouseDown:
> switch (aButton) {
> case WidgetMouseEvent::eLeftButton:
> pluginEvent.event = WM_LBUTTONDOWN;
> break;
> +
> case WidgetMouseEvent::eMiddleButton:
> pluginEvent.event = WM_MBUTTONDOWN;
> break;
> +
> case WidgetMouseEvent::eRightButton:
> pluginEvent.event = WM_RBUTTONDOWN;
> break;
> +
> default:
> break;
> }
> break;
> +
> case eMouseUp:
> switch (aButton) {
> case WidgetMouseEvent::eLeftButton:
> pluginEvent.event = WM_LBUTTONUP;
> break;
> +
> case WidgetMouseEvent::eMiddleButton:
> pluginEvent.event = WM_MBUTTONUP;
> break;
> +
> case WidgetMouseEvent::eRightButton:
> pluginEvent.event = WM_RBUTTONUP;
> break;
> +
> default:
> break;
> }
> break;
> +
> case eMouseDoubleClick:
> switch (aButton) {
> case WidgetMouseEvent::eLeftButton:
> pluginEvent.event = WM_LBUTTONDBLCLK;
> break;
> +
> case WidgetMouseEvent::eMiddleButton:
> pluginEvent.event = WM_MBUTTONDBLCLK;
> break;
> +
> case WidgetMouseEvent::eRightButton:
> pluginEvent.event = WM_RBUTTONDBLCLK;
> break;
> +
> default:
> break;
> }
> break;
> +
> case eMouseMove:
> pluginEvent.event = WM_MOUSEMOVE;
> break;
> +
> case eMouseExitFromWidget:
> pluginEvent.event = WM_MOUSELEAVE;
> break;
> +
> default:
> pluginEvent.event = WM_NULL;
> break;
> }
So, please cancel inserting empty lines into this switch statement too.
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261774
::: widget/windows/nsWindow.cpp:4558
(Diff revision 1)
> + // Fire an observer when the user initially touches a touch screen. Front end
> + // uses this to modify UX.
> + if (sTouchInputActiveState != eTouch) {
> + sTouchInputActiveState = eTouch;
> + nsCOMPtr<nsIObserverService> obsServ =
> + mozilla::services::GetObserverService();
Do we need |mozilla::| here?
::: widget/windows/nsWindow.cpp:4570
(Diff revision 1)
> + if (TouchEventShouldStartDrag(aEventMessage, eventPoint)) {
> + event.mMessage = aEventMessage = eMouseTouchDrag;
Well, you change this behavior.
In the former code, aEventMessage is modified from eMouseDown or eMouseDoubleClick to eMouseTouchDrag if it returns true and then, above switch-case don't do nothing for them. However, new code will do something when eMouseDown, and then, it'll be modified here.
Are you sure this is safe change?
::: widget/windows/nsWindow.cpp:4581
(Diff revision 1)
> + } else {
> + // Fire an observer when the user initially uses a mouse or pen.
> + if (sTouchInputActiveState != ePrecise) {
> + sTouchInputActiveState = ePrecise;
> + nsCOMPtr<nsIObserverService> obsServ =
> + mozilla::services::GetObserverService();
Do we really need |mozilla::| here?
::: widget/windows/nsWindow.cpp:4700
(Diff revision 1)
> +
> + switch (aEventMessage) {
> + case eMouseDown:
> + CaptureMouse(true);
> + break;
> +
nit: unnecessary empty line.
::: widget/windows/nsWindow.cpp:4713
(Diff revision 1)
> + // No button pressed.
> + !(wParam & (MK_LBUTTON | MK_MBUTTON | MK_RBUTTON))) {
> + CaptureMouse(false);
> + }
> + break;
> +
nit: unnecessary empty line.
::: widget/windows/nsWindow.cpp:4752
(Diff revision 1)
> - } else if (aEventMessage == eMouseExitFromWidget) {
> + case eMouseExitFromWidget:
> - if (sCurrentWindow == this) {
> + if (sCurrentWindow == this) {
> - sCurrentWindow = nullptr;
> + sCurrentWindow = nullptr;
> - }
> + }
> + break;
> +
nit: We don't need this empty line.
::: widget/windows/nsWindow.cpp:4765
(Diff revision 1)
> // the context menu key code in EventListenerManager::HandleEvent()
> // released it already.
> return result;
> }
>
> + /* * * * * */
Odd comment, please remove this.
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261782
::: widget/windows/nsWindow.cpp:5985
(Diff revision 1)
> - case WM_NCLBUTTONDBLCLK:
> - DispatchMouseEvent(eMouseDoubleClick, 0, lParamToClient(lParam),
> - false, WidgetMouseEvent::eLeftButton,
> - MOUSE_INPUT_SOURCE());
> - result =
> - DispatchMouseEvent(eMouseUp, 0, lParamToClient(lParam),
> - false, WidgetMouseEvent::eLeftButton,
> - MOUSE_INPUT_SOURCE());
> - DispatchPendingEvents();
> - break;
> -
> +//FIXME @ h-h & reviewers: Unlike the M and R button equivalents, the `WM_ NC L BUTTON DBLCLK` event has no `WM_ NC L BUTTON DOWN` and `WM_ NC L BUTTON UP` companions in this switch block; and its code looks different to the other deleted code in this switch block. Can it be removed safely?
> +// case WM_NCLBUTTONDBLCLK:
> +// DispatchMouseEvent(eMouseDoubleClick, 0, lParamToClient(lParam),
> +// false, WidgetMouseEvent::eLeftButton,
> +// MOUSE_INPUT_SOURCE());
> +// result =
> +// DispatchMouseEvent(eMouseUp, 0, lParamToClient(lParam),
> +// false, WidgetMouseEvent::eLeftButton,
> +// MOUSE_INPUT_SOURCE());
> +// DispatchPendingEvents();
> +// break;
> +//
I think that we need to add WM_NCLBUTTONDOWN and WM_NCLBUTTONUP message cases for removing this case. But I don't know who listens to these non-client mouse event messages.
It was implemented by:
https://hg.mozilla.org/mozilla-central/rev/f5d0530311c0ee758346aa4bee79417d140ff1c0
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
https://reviewboard.mozilla.org/r/254770/#review261784
Finally, it seems that you can create automated test to test the click count with various cases, e.g., changing element at same position, etc.
Native mouse event can be synthesized with nsIDOMWindowUtils::sendNativeMouseEvent() <https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/interfaces/base/nsIDOMWindowUtils.idl#485-501>. So, perhaps, mochitest-chrome is the best framework. You should create widget/tests/test_click_count.xul or something, then, it should open child window with widget/tests/window_click_count.html since the test_*.(xul|html) is loaded in <iframe> which is not so large. Therefore, it should create larger and fixed size window to test. Then, nsIDOMWindowUtils can be retrieved with SpecialPowers.getDOMWindowUtils() and can synthesize native mousedown/mouseup events with the API. You need to register new tests into widget/tests/chrome.ini. Then, you need to add |skip-if = toolkit != "windows" && toolkit != "cocoa"| for making your test run only on Windows and macOS. If you have neither tryserver permission nor Mac, I'll post your patches into tryserver when you attached new patch(es) into this bug.
This document must help you: <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Chrome_tests>
Although it'd be really nicer if you could write the test as mochitest-plain since it's tested with content-process but I'm not sure if it's possible. Looks like that existing tests which use the API are mochitest-chrome.
Ah, but nsWindow directly refers GetDoubleClickTime() and nsChildView always synthesizes native event with click count 1:
https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/widget/cocoa/nsChildView.mm#1138
and it refers the click count:
https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/widget/cocoa/nsChildView.mm#4604-4614
So, it is impossible to test with nsIDOMWindowUtils.sendMouseEvent() for now. Please ignore the previous comment, sigh.
Comment 21•6 years ago
|
||
This is more sophisticated than the "testcase" attachment. It lets you choose the desired event types including `mousedown` and `mouseup`.
Comment 22•6 years ago
|
||
Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)
I'll leave the code review for Masayuki, but I can comment on my original patch.
The issue at the time was that the glass area needs to be made a non-client area in order for Aero Snap to work (and more generally, for the OS to handle the window dragging rather than us).
However, we treat {right, middle, double} clicks from the non-client area as a normal click in order to keep other functionality working (our own context menu instead of the OS, and opening new tabs).
The reason for the DBLCLICK + MOUSE UP in that patch are explained on bug 575248, comments 55 to 57.
So you just need to ensure that with your changes, the window can still be dragged by Aero Snap in the empty area, but a double click or a right click in that same empty area still works. If this all works, I'm fine with whatever changes you make here
Attachment #8989796 -
Flags: review?(felipc)
Comment 23•6 years ago
|
||
@ Olli Pettay [:smaug]
@ Jim Mathies [:jimm]
@ :Felipe Gomes
Unfortunately and strangely, the straightforward way of solving this bug makes the browser window undraggable (unmoveable). Dragging in the client area still works (tested with https://www.draw.io/).
:Felipe Gomes wrote 8 years ago:
> Jim, talking with gavin/smaug the other day on #developers this was the approach suggested to raise a double click event from widget code. The NS_MOUSE_DOUBLECLICK will set clickCount = 2 and dispatch a mousedown, and the following NS_MOUSE_UP inherits the clickCount and then the EventStateManager generates a doubleclick.
> ...
> The NS_MOUSE_DOUBLECLICK message is just converted to a mousedown event with clickcount = 2 on DispatchMouseEvent. But the ESM [EventStateManager] only triggers the double click event after receiving the subsequent mouseup event too. There's no way to tell the ESM directly to dispatch a {double}click event.
> [https://bugzilla.mozilla.org/show_bug.cgi?id=575248#c55]
Please view this code (the `case`): https://hg.mozilla.org/mozilla-central/file/tip/widget/windows/nsWindow.cpp#l5938 .
It was introduced with this commit: https://hg.mozilla.org/mozilla-central/rev/f5d0530311c0 .
This very much seems to me to be a dirty hack. I may be able to solve this bug another way, but I would like to be able to do it by turning off Windows' double-click recognition. Would it be possible to solve the problems quoted above by passing-on to the JavaScript side the information, whether the mouse event happened in the non-client area of the window? Or would that introduce problems with Linux and macOS?
Comment 24•6 years ago
|
||
Related?
> if (NS_SUCCEEDED(ret) && aEvent->mClickCount == 2 &&
> mouseContent && mouseContent->IsInComposedDoc()) {
> //fire double click
> ret = InitAndDispatchClickEvent(aEvent, aStatus, eMouseDoubleClick,
> presShell, mouseContent, currentTarget,
> notDispatchToContents,
> aOverrideClickTarget);
> }
> [https://hg.mozilla.org/mozilla-central/file/tip/dom/events/EventStateManager.cpp#l5058]
Assignee | ||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•