550528 - Click count (event.detail) is different compared to Opera and Chrome when quadruple (or more) clicking

Status: REOPENED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

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

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

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.

Comment 2

[Neil Deakin]

What does OSX output on Mozilla?

Comment 3

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

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

[danya.postfactum]

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

[danya.postfactum]

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.

Comment 6

[h-h]

Attachment: Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)

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.

Also, Opera and Chrome allow for infinite grouping of clicks, at least on
Windows.

Until now, on Windows, only double- and triple-clicks are recognized, which
affects selection behavior and renders the config option
`about:config?filter=browser.triple_click_selects_paragraph` useless. When
`false`, quadruple-clicks should select the paragraph. (Use, e.g., Wikipedia
articles for testing.) Furthermore, it prevents us from defining behavior for
quintuple-, sextuple-clicks etc.

Not only does `nsWindow.cpp` restrict the maximum amount of grouped clicks, but
it also contains quite a number of out-of-date and wrong comments in the
respective section. Hence, I also restructed the method that does the click
grouping for more clarity. I created sections because it is a very long method.
(Changing the order of code blocks was necessary anyway, because
`TouchEventShouldStartDrag()` expects `eMouseDoubleClick`, if applicable, which,
with this patch, Windows does not send anymore, but we have our own recognition
that changes `eMouseDown` to `eMouseDoubleClick`.)

At first, please take a general look at the changes and answer my questions in
the `FIXME` texts. Then, I think, it would be best that I rebase the changes by
means of a temporary Git repository to create a series of commits that you can
review easier to verify that I didn't make mistakes in this important piece of
code.

Review commit: https://reviewboard.mozilla.org/r/254770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254770/

Comment 7

[h-h]

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 8

[h-h]

Comment on attachment 8989796 [details]
Bug 550528 - Correct click grouping on Windows (like for quadruple-clicks)

See commit message.

Comment 9

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

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.

Comment 10

[h-h]

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

[h-h]

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!

Comment 12

[h-h]

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!

Comment 13

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

(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

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

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.

Comment 15

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

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

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

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

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

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

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

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

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

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.

Comment 20

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

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

[h-h]

Attachment: click-counter.html

This is more sophisticated than the "testcase" attachment. It lets you choose the desired event types including `mousedown` and `mouseup`.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

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

Comment 23

[h-h]

@ 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

[h-h]

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]