Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 8 years ago

https://wicg.github.io/auxclick/
that isn't really a spec but a random wicg doc, but blink is about to ship it, and I think it is sane.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 1 • 8 years ago

Interesting. Should we stop dispatching click event for non-primary buttons when we implement it? (I guess so, but it might cause breaking some add-ons...)

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 2 • 8 years ago

I think we should first implement auxclick and then start warning about use of click for non-primary and then eventually remove non-primary clicks.

	Andrew Overholt [:overholt]
	Comment 3 • 8 years ago

But I like "opening a new tab when middle clicking on a link"! :)

(Should this be higher priority than P1 given Blink shipping it soon?)

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 4 • 8 years ago

This could be and I think implementation could be very easy, just dispatch auxclick whenever we dispatch click event for non-primary button. So, in EventStateManager add something to the method which dispatches click and dblclick

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 5 • 8 years ago

This should be rather simple to do, especially if we initially still support dispatching click events for middle/right too and remove those in some other bug.

Kevin, by any chance, would you be interested in to take a look at this?

	Kevin Wern Assignee
	Comment 6 • 8 years ago

Yes. I should be able to look at this in the next few days.

Let me know if there's any additional info you think would help.

	Kevin Wern Assignee
	Comment 8 • 8 years ago

As mentioned in the commit message, I am dispatching to all eligible content nodes because otherwise the behavior described in the design doc doesn't work. Is there anything I should be looking out for?

	Kevin Wern Assignee
	Comment 9 • 8 years ago

Additionally, what would be best method to test this (i.e. simulating clicks)? I tried doing something with synthesizeMouse() but was running into some trouble.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 8 years ago

synthesizeMouse should work. Click is generated when mousedown+mouseup is dispatched

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8815946 [details]
Bug 1304044 - implement auxclick

https://reviewboard.mozilla.org/r/96718/#review97178

couple of small things and need to get some tests.

::: dom/events/EventStateManager.cpp:4683
(Diff revision 1)
> +        auxClickEvent.mClickCount = aEvent->mClickCount;
> +        auxClickEvent.mModifiers = aEvent->mModifiers;
> +        auxClickEvent.buttons = aEvent->buttons;
> +        auxClickEvent.mTime = aEvent->mTime;
> +        auxClickEvent.mTimeStamp = aEvent->mTimeStamp;
> +        auxClickEvent.mFlags.mNoContentDispatch = true;

This is wrong. mNoContentDispatch should be false since we want all the elements to be able to get this event.

Clearly this needs some tests, where event listeners are in elements, document and window and all them should get the event.

::: dom/events/EventStateManager.cpp:4685
(Diff revision 1)
> +        auxClickEvent.buttons = aEvent->buttons;
> +        auxClickEvent.mTime = aEvent->mTime;
> +        auxClickEvent.mTimeStamp = aEvent->mTimeStamp;
> +        auxClickEvent.mFlags.mNoContentDispatch = true;
> +        auxClickEvent.button = aEvent->button;
> +        auxClickEvent.inputSource = aEvent->inputSource;

Could you possibly add some helper method to initialize and dispatch events. We have now 3 times pretty much the same code here.

::: dom/html/HTMLInputElement.cpp:4710
(Diff revision 1)
>  
>          } break; // eKeyPress || eKeyUp
>  
>          case eMouseDown:
>          case eMouseUp:
> +        case eMouseAuxClick:

This looks wrong. Why would we need any auxclick handling here?

::: layout/base/AccessibleCaretEventHub.cpp:520
(Diff revision 1)
>    nsPoint point = GetMouseEventPosition(aEvent);
>  
>    if (aEvent->mMessage == eMouseDown ||
>        aEvent->mMessage == eMouseUp ||
>        aEvent->mMessage == eMouseClick ||
> +      aEvent->mMessage == eMouseAuxClick ||

Why we need this. eMouseAuxClick is closer to eContextMenu and that isn't handled here.

	Kevin Wern Assignee
	Comment 12 • 8 years ago
mozreview-review-reply

Comment on attachment 8815946 [details]
Bug 1304044 - implement auxclick

https://reviewboard.mozilla.org/r/96718/#review97178

> This is wrong. mNoContentDispatch should be false since we want all the elements to be able to get this event.
> 
> Clearly this needs some tests, where event listeners are in elements, document and window and all them should get the event.

Yeesh...I swear I tested it toggling between false and notDispatchToContents. Must have set it wrong before finally sending the patch. Sorry about that.

> Why we need this. eMouseAuxClick is closer to eContextMenu and that isn't handled here.

I thought the intended group here was "all mouse events that aren't mouse movements." I'm not sure why eContextMenu isn't here--unless it was considered redundant with the other events.

	Kevin Wern Assignee
	Comment 14 • 8 years ago

Added a test, created a helper function, and removed those points you gave feedback about.

I'll probably need some help with handling other parts in the code, because I'm a bit confused regarding your feedback. I originally based my handling on where dblclick was handled (because it seemed logically similar--an event triggered alongside clicks), but your comment makes sense with contextmenu. I'm just not sure if this means that a fair amount of these adjustments to the code (because they are not alongside eContextMenu) are also invalid, or if they are valid for a different reason.

Basically, I'm not sure if your feedback there applied to that particular code fragment or the entire scope of those other adjustments.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8815946 [details]
Bug 1304044 - implement auxclick

https://reviewboard.mozilla.org/r/96718/#review97834

::: dom/events/EventStateManager.cpp:4687
(Diff revision 2)
>        nsWeakFrame currentTarget = mCurrentTarget;
> -      ret = presShell->HandleEventWithTarget(&event, currentTarget,
> -                                             mouseContent, aStatus);
> +      ret = InitAndDispatchClickEvent(aEvent, aStatus, eMouseClick,
> +                                      presShell, mouseContent, currentTarget,
> +                                      notDispatchToContents);
> +
> +      if (NS_SUCCEEDED(ret) && mouseContent && fireAuxClick &&

Could you move auxclick dispatching to happen after doubleclick. I think that would give better consistency.

::: dom/events/test/test_bug1304044.html:29
(Diff revision 2)
> +      if (node.nodeName)
> +        return node.nodeName;
> +      return node;
> +    }
> +
> +    function Event(listener, target) {

Calling this "Event" is a tad misleading, given that the global already has window.Event which is totally different beast. Maybe call this TargetAndListener ?

	Kevin Wern Assignee
	Comment 17 • 8 years ago

Trying to do a try run, but I'm getting an access denied error... I'll give it a go tomorrow, then if that works I'll resolve all the issues and mark this for checkin.

	Kevin Wern Assignee
	Comment 18 • 8 years ago

Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f140162cae1d376247a7b3013adcbf58651e3019

	Kevin Wern Assignee
	Comment 19 • 8 years ago

Looks fine, so marking for checkin.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 8 years ago

Autoland couldn't rebase this for landing.

	Kevin Wern Assignee
	Comment 22 • 8 years ago

Rebased against mozilla-inbound. Should work now.

	Pulsebot
	Comment 23 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea7338a0b3de
implement auxclick r=smaug

	Phil Ringnalda (:philor)
	Comment 24 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ea7338a0b3de

	Rick Byers
	Comment 25 • 7 years ago

Thanks for shipping this!  Once there's two implementations in the wild we should be able to get auxclick moved out of incubation into the UI events spec for formal standardization.

Note that in blink we stopped sending click at the same time as we added auxclick because we wanted to avoid potential compat issues with double handling (a site having both a click and auxclick listener).  The compat fallout of sites depending on click for middle/right button has been minimal so in our case I feel confident it was the right choice.

	Chris Mills [:cmills]
	Comment 26 • 7 years ago

I've documented this...

New pages have been added for the handler and event:

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onauxclick
https://developer.mozilla.org/en-US/docs/Web/Events/auxclick

Browser compat info and links have been added where appropriate:

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers
https://developer.mozilla.org/en-US/docs/Web/Events

I updated descriptions of other events to mention that soon they'll only fire on the primary button:

https://developer.mozilla.org/en-US/docs/Web/Events/mousedown
https://developer.mozilla.org/en-US/docs/Web/Events/click
https://developer.mozilla.org/en-US/docs/Web/Events/dblclick
https://developer.mozilla.org/en-US/docs/Web/Events/mouseup

I created a crappy little demo:

https://mdn.github.io/dom-examples/auxclick/

I also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

Let me know if it all looks OK. Thanks!

	Navid Zolghadr
	Comment 27 • 7 years ago

Chris, I don't think this would apply to mousedown and mouseup events.
Only click and dbclick events will soon not be fired for non-primary buttons.

	Chris Mills [:cmills]
	Comment 28 • 7 years ago

(In reply to Navid Zolghadr from comment #27)
> Chris, I don't think this would apply to mousedown and mouseup events.
> Only click and dbclick events will soon not be fired for non-primary buttons.

ah cool, thanks for catching that. I've updated the wording.

	:Gijs (he/him)
	Comment 29 • 6 years ago

(In reply to Olli Pettay [:smaug] (please try to find other reviewers for non-web components patches) from comment #2)
> I think we should first implement auxclick and then start warning about use
> of click for non-primary and then eventually remove non-primary clicks.

Did we ever start warning here? When do you think we should remove the non-primary click "click" event dispatching, considering Chrome shipped that with Chrome 55 (just over a year ago) ?

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 30 • 6 years ago

We did not, and now that I think, I don't know how to warn.