1245401 - Can no longer prevent window dragging dynamically from an add-on

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[tabmix.onemen]

User can drag the window by clicking (and holding the mouse) on the title bar or on empty area of the toolbars. Dragging is also possible with modifiers - shift, ctrl, meta, alt.

This however prevents the use of mouse click options on empty area of TabsToolbar for example.

Mouse left click with modifiers shouldn't start window dragging, same as middle-click right-click and double-click.

Before bug 1219215, it was possible to call event.stopPropagation when "MozMouseHittest" was triggered with modifiers

Comment 1

[:Gijs (he/him)]

On a vanilla copy of Firefox 45 (so without the patches from bug 1219215) on Windows 8, I can drag the window by the tabs toolbar using ctrl- or alt-drag. It seems to me that this is expected? Are you basically saying you need this for tabmix plus? For what functionality?

Is setting -moz-window-dragging: no-drag on the <tabs> element an alternative? (It seems that that is where you're listening for mozmousehittest events right now, looking at the add-on code in MXR.) I guess that would break dragging "normally" (ie without modifiers) using the empty space on the tabs toolbar...

This is a little tricky because it seems like always preventing this behaviour (modifier-drag) even when TMP is not installed is not OK. Adding another CSS value would complicate the layout code significantly. We could add back the mozmousehittest code but it would only be used by add-ons which seems likely it'd break in the future when we forget or whatever. Plus we'd pay a perf cost for sending those events all the time while dragging tabs etc. Part of the nice thing about 1219215 is that that is now gone, and I would prefer not to put it back.

The other interesting thing is that looking at https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/widget/windows/nsWindow.cpp#6043-6057 , I don't see where it even used to put in the correct modifier states, which it does elsewhere using code like this: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/widget/windows/nsWindow.cpp#4174-4175 .

We could potentially/maybe only dispatch the event when modifier keys are pressed to save ourselves some perf cost, but that feels even hackier. :-\

If people have other ideas, I'd love to hear them.

Comment 2

[tabmix.onemen]

(In reply to :Gijs Kruitbosch from comment #1)
> On a vanilla copy of Firefox 45 (so without the patches from bug 1219215) on
> Windows 8, I can drag the window by the tabs toolbar using ctrl- or
> alt-drag. It seems to me that this is expected?
I don't find it as expected as you.

Tabmix add preferences that allow the users to choose command to perform when clicking on tab or on empty space on TabsToolbar. These options are only for double-click, middle-click and left click with modifiers.
See http://tabmixplus.org/support/viewpage.php?t=3&p=mouse-clicking
Setting -moz-window-dragging: no-drag for TabsToolbar is working without a problem.  I can add this to my css file:
> +#main-window[tabsintitlebar] #TabsToolbar[tabmix-disallow-drag] {
> +  -moz-window-dragging: no-drag;
> +}

However this also prevents the default drag behavior, when user click on TabsToolbar without modifiers.
If it is not to complicating I prefer an option to prevent window dragging dynamically, when user click on TabsToolbar with modifiers.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

One alternative would be to convert WM_NCLBUTTON* messages *with modifiers* into effectively the WM_LBUTTON* counterparts. If it's acceptable to let unmodified left-click to be unpreventable, I think this could work easily  (probably even the unmodified ones but I'd like to avoid that).

There's precedent for that. The reason that middle/right click works is because we do the same for them, in order to let features like "Middle click on empty space to open new tab" to work. See how these messages dispatch a normal mouse event: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp?rev=75dfe10ec44a#5271

Then, Tabmix would just have to call preventDefault() on those events to avoid dragging, if it's gonna handle the modifier. But I bet this is even less complexity than what it had to do MozMouseHittest.

(* take this with a grain of salt because i'm only 80% sure it will work.. but I think it's worth a try!)

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1245401 - send mousedown/mouseup/mousemove events to non-client area, but only when modifiers are pressed, r?felipe

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

Comment 5

[:Gijs (he/him)]

Felipe, let me know if you're not comfortable reviewing this and I need to find someone else. :-)

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8715348 [details]
MozReview Request: Bug 1245401 - send mousedown/mouseup/mousemove events to non-client area, but only when modifiers are pressed, r?felipe

https://reviewboard.mozilla.org/r/33391/#review30117

::: widget/windows/nsWindow.h:218
(Diff revision 1)
> +  virtual bool            DispatchLeftMouseIfModifiersUsed(UINT aMessage, LPARAM lParam);

no need for `virtual`. You could define this a little further below to be inside the `/* Event helpers */` section

::: widget/windows/nsWindow.cpp:5187
(Diff revision 1)
> +      if (DispatchLeftMouseIfModifiersUsed(msg, lParam)) {
> +        result = true;
> +      }

why not just `result = DispatchLeft...`?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8715348 [details]
MozReview Request: Bug 1245401 - send mousedown/mouseup/mousemove events to non-client area, but only when modifiers are pressed, r?felipe

This doesn't actually work - it breaks modifier-drag when no listeners are attached (ie 'normal' behaviour).

Per discussion with felipe, I'll look at just bringing back the mozmousehittest only when modifiers are active, or if we're in mouse capture mode tomorrow. Either that or wontfix. :-\

Comment 8

[:Gijs (he/him)]

I think this can be worked around by TMP:

let modifierDown = false;
let up = e => {
  if (modifierDown && !e.shiftKey && !e.ctrlKey && !e.altKey) {
    document.getElementById("TabsToolbar").style.removeProperty("-moz-window-dragging");
  }
};

let dn = e => {
  if (e.shiftKey || e.ctrlKey || e.altKey) {
    modifierDown = true;
    document.getElementById("TabsToolbar").style.MozWindowDragging = "no-drag";
  }
};

window.addEventListener("keydown", dn, true);
window.addEventListener("keyup", up, true);

onemen, does that work for you? Because the hacks we'd need to fix this on the Gecko side are quite substantial, and this seems like a much cleaner solution.

Comment 9

[tabmix.onemen]

(In reply to :Gijs Kruitbosch from comment #8)
> I think this can be worked around by TMP:
> .....
> window.addEventListener("keydown", dn, true);
> window.addEventListener("keyup", up, true);
> 
> onemen, does that work for you? Because the hacks we'd need to fix this on
> the Gecko side are quite substantial, and this seems like a much cleaner
> solution.

Great idea, it works perfectly.

Comment 10

[:Gijs (he/him)]

Resolving per comment #9. Thanks everyone! :-)