Closed Bug 1304044 Opened 7 years ago Closed 7 years ago

Implement auxclick

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
50 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: smaug, Assigned: kevin.m.wern)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1304044 - implement auxclick
58 bytes, text/x-review-board-request
smaug
: review+
Details 
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.
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...)
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.
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?)
Flags: needinfo?(bugs)
Priority: -- → P3
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
Flags: needinfo?(bugs)
Depends on: 968265
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?
Flags: needinfo?(kevin.m.wern)
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.
Flags: needinfo?(kevin.m.wern)
Assignee: nobody → kevin.m.wern
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?
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.
synthesizeMouse should work. Click is generated when mousedown+mouseup is dispatched
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.
Attachment #8815946 - Flags: review?(bugs) → review-
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.
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.
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 ?
Attachment #8815946 - Flags: review?(bugs) → review+
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.
Looks fine, so marking for checkin.
Keywords: checkin-needed
Autoland couldn't rebase this for landing.
Keywords: checkin-needed
Rebased against mozilla-inbound. Should work now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea7338a0b3de
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Keywords: dev-doc-needed
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, 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.
(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.
See Also: → 1379466
(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) ?
Flags: needinfo?(bugs)
We did not, and now that I think, I don't know how to warn.
Flags: needinfo?(bugs)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.