327893 - Should use defaultPrevented for Autoscrolling to allow others to capture middle-clicks

Status: NEW

(Toolkit :: UI Widgets, defect)

Description

[nrlz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

Currently the autoscroll feature will activate when middle-clicked, unless if the target is a link or an input element. These things are hardcoded in "global/content/bindings/browser.xml::isAutoscrollBlocker()". Because the autoscroll conditions are hardcoded, this makes it hard for website designers and extension authors to extend the middle-click button for certain web content.

For example, say I wanted to write an extension that manipulates an SVG image when middle-clicked, I cannot just add an event listener, because in addition to my listener being called, the page will also scroll.

A possible solution is to use getPreventDefault() to test if a programmer wants to cancel the event to add his/her own functionality:

== global/content/bindings/browser.xml ==:

  <handler event="mousedown">
    <![CDATA[
      if (this._isScrolling) {
        stopScroll();
-     } else if (event.button == 1) {
+     } else if (event.button == 1 && !event.getPreventDefault()) {
        if (!this.autoscrollEnabled  ||
            this.currentURI.spec == "about:blank"|| 
            this.isAutoscrollBlocker(event.originalTarget))
          return;
        this.addEventListener('mousemove', this.handleMouseMove, false);
  .....
====

Reproducible: Always

Steps to Reproduce:
1. Try testcase

Comment 1

[nrlz]

Attachment: testcase for getPreventDefault()

Comment 2

[Peter Kasting]

WebKit has implemented this: As of December 2009, calling preventDefault() on the mousedown event for a middle click will prevent middle-mouse autoscroll from triggering.

After some discussion with web developers here at Google we decided that WebKit's behavior was preferable and Gecko should change to match it, if possible.

Comment 3

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

Neil, you've done some work related to autoscrolling, right?

Comment 4

[[e-mail]]

This is obviously technically feasible. Bug 242621 changed autoscroll to capture the mouse for some reason not documented in the bug :-(

I'd like to point out that we have a pref to ignore preventDefault() on the contextmenu event, should we have a pref to ignore it on autoscroll too?

Comment 5

[Peter Kasting]

(In reply to comment #4)
> I'd like to point out that we have a pref to ignore preventDefault() on the
> contextmenu event, should we have a pref to ignore it on autoscroll too?

I don't think it's necessary.  Authors prevent the context menu to do arguably-annoying things like disabling "copy".  The only reason to prevent autoscroll is if the site really needs the middle button to do something different, in which case we probably want to allow this.

So I'd implement it without a pref and add a pref someday if sites start to use this in a user-hostile way.

Comment 6

[Jan Wolter]

As an author of various Javascript games, I'd love it if Firefox would be willing to release control of the middle mouse button. In WebKit, IE and Konqueror the default actions of middle clicks can be cancelled, but not in Firefox or (last I checked) Opera. Javascript applications can catch the mousedown event, but cannot prevent the default action from occurring after they return.

The patch posted five years ago by nrlz above still works with only minor modificaton:

--- a/toolkit/content/widgets/browser.xml
+++ b/toolkit/content/widgets/browser.xml
@@ -1248,17 +1248,17 @@
           try {
             this.mPrefs.setBoolPref("accessibility.browsewithcaret",!browseWithCaretOn);
           } catch (ex) {
           }
         ]]>
       </handler>
       <handler event="mousedown" phase="capturing">
         <![CDATA[
-          if (!this._scrollable && event.button == 1) {
+          if (!this._scrollable && event.button == 1 && !event.getPreventDefault()) {
             if (!this.autoscrollEnabled ||
                 this.isAutoscrollBlocker(event.originalTarget))
               return;

             this.startScroll(event);
           }
         ]]>
       </handler>

However, this doesn't really do the whole job. There are various other default actions associated with the middle mouse button in Firefox (pasting URLs if middlemouse.contentLoadURL is true, opening links in new windows if you click on a link) which also cannot be prevented. As far as I can tell (which admittedly isn't far) these are triggered via an entirely different mechanism. My best guess is that they are tied into the "click" event rather than the "mousedown" event, and can't be canceled because the click event on the middle mouse button isn't ever actually dispatched to the user Javascript. That's kind of the way the code I saw looked, and I notice that the "open a link in a new tab" function only fires if you mouseup and mousedown on the same link, which sure looks like a "click" event handler running.

There are some comments in the code about not dispatching click events for buttons other than the left button because of IE compatibility. But IE does fire click events on middle mouse clicks and always has (though for some reason, they don't fire dblclick events -- WebKit fires both).  Firefox isn't the only browser that doesn't fire click events on middle clicks. -- it has Opera and Konqueror for company.  But if the easiest way to fix the default prevention problem were to dispatch the middle click events, so that they could be cancelled by an onclick handler if desired, then I don't think that that would cause any compatibility problems -- quite the contrary, it would bring Firefox more in line with other browsers.

Comment 7

[Jan Wolter]

Well, the comments I saw about IE compatibility were talking about something else, but I found a simple fix that seems to do what I want:


--- a/content/events/src/nsEventStateManager.cpp
+++ b/content/events/src/nsEventStateManager.cpp
@@ -4226,17 +4226,17 @@ nsEventStateManager::CheckForAndDispatch
     if (aEvent->widget) {
       PRBool enabled;
       aEvent->widget->IsEnabled(&enabled);
       if (!enabled) {
         return ret;
       }
     }
     //fire click
-    if (aEvent->button == nsMouseEvent::eMiddleButton ||
+    if (//aEvent->button == nsMouseEvent::eMiddleButton ||
         aEvent->button == nsMouseEvent::eRightButton) {
       flags |=
         sLeftClickOnly ? NS_EVENT_FLAG_NO_CONTENT_DISPATCH : NS_EVENT_FLAG_NONE;
     }

     nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_CLICK, aEvent->widget,
                        nsMouseEvent::eReal);
     event.refPoint = aEvent->refPoint;

This change means (1) a "click" event is received by the content Javascript on middle click events, and (2) the content Javascript can cancel the default actions that are attached to the middle click event, like opening links in a new  window or pasting URLs.

Of course, having only looked at the Mozilla source for the first time this morning, I have no idea what other effects this little change may have. What I can say is that the two effects listed above are imminently desirable.

Comment 8

[Jan Wolter]

Attachment: A patch to allow middle mouse events to be cancelled with preventDefault

Comment 9

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

Comment on attachment 560590 [details] [diff] [review]
A patch to allow middle mouse events to be cancelled with preventDefault

I'll try to look at the patch and issue next week when I'm back home.
Please ping me here, or on IRC if I forget.

(It would be great to get rid of NS_EVENT_FLAG_NO_CONTENT_DISPATCH, but
that would be a bigger task.)

Comment 10

[[e-mail]]

Comment on attachment 560590 [details] [diff] [review]
A patch to allow middle mouse events to be cancelled with preventDefault

Drive-by review: browser.xml captures the mouse event, so it will start scrolling before the page sees it. Maybe changing phase="capturing" to group="system" is the best way to fix that.

Comment 11

[Masatoshi Kimura [:emk]]

> !event.getPreventDefault()) {
Tt should be defaultPrevented instead of the proprietary getPreventDefault().

Comment 12

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

Comment on attachment 560590 [details] [diff] [review]
A patch to allow middle mouse events to be cancelled with preventDefault

Neil's and Masatoshi's comments are very valid. And this needs tests.

Also commenting out some code isn't quite right.
Just remove it :)

Comment 13

[Masatoshi Kimura [:emk]]

Please do not add any new getPreventDefault user.