Closed Bug 1533630 Opened 5 years ago Closed 4 years ago

Gecko fires click events for middle and right clicks, against the uievents spec.

Categories

(Core :: DOM: Events, defect, P2)

Product:
Core
Component:
DOM: Events
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox66 --- wontfix
firefox67 --- fix-optional
firefox68 --- affected

People

(Reporter: twisniewski, Assigned: jdai)

References

(
URL
)

Details

(Whiteboard: [webcompat])

Attachments

(2 files, 1 obsolete file)

test.html
253 bytes, text/html
Details
Bug 1533630 - Shouldn't fire click events for middle and right clicks;
47 bytes, text/x-phabricator-request
Details | Review
Attached file test.html (obsolete) —

Firefox allow an indirect middle-click event on a anchor (detected via the document, rather than the anchor itself) to be defaultPrevented, which is does not allow with a direct handler on the link itself. Chrome does not allow either case to be defaultPrevented.

This is causing the breakage of middle-click behavior on the New York Times subscribe and login and other buttons, because they have a React component which purposely stops the default behavior of clicks and redirects the page to the link's target:

var ExitLink = function ExitLink(_ref) {
  var url = _ref.url,
      returnUrl = _ref.returnUrl,
      children = _ref.children,
      linkProps = (0, _objectWithoutProperties2.default)(_ref, ["url", "returnUrl", "children"]);

  var onClick = function onClick(e) {
    e.preventDefault();

    if (linkProps.onClick) {
      linkProps.onClick(e);
    }

    window.location.href = createReturnUrl(url, returnUrl);
  };

  return _react.default.createElement("a", (0, _extends2.default)({
    href: url
  }, linkProps, {
    onClick: onClick
  }), children);
};

I suspect that we would likely want to prevent this breakage, given that we do not allow more direct event handlers to prevent the middle click, but either way it's a noticable interop difference, as this issue was reported at https://webcompat.com/issues/27378

I've attached a trivial test-case which demonstrates the behavior difference, in case it helps.

Flags: webcompat?

Seems duplication of bug 1249922, bug 1249970 and bug 1250652

It may be a dupe, but I don't think it's a dupe of those precise bugs. But thanks for mentioning those bugs, as based on bug 1249922 comment 2, this is definitely a Gecko webcompat bug:

The click event should only be fired for the primary pointer button (i.e., when button value is 0, buttons value is 1). Secondary buttons (like the middle or right button on a standard mouse) MUST NOT fire click events. See auxclick for a corresponding event that is associated with the non-primary buttons.

I'll re-title this to reflect that, just in case there is no bug to dupe this against.

Summary: Different DOM event behavior on middle-clicks compared to Blink. → Gecko fires click events for middle and right clicks, against the uievents spec.

In the testcase on Nightly67.0a1 windows10,

When left mouse click on the link:
Web console shows "click { target: a, buttons: 0, clientX: 43, clientY: 25, layerX: 43, layerY: 25 } attachment.cgi:9:83"
i.e, click event fired.

When middle(or right) mouse click on the link:
There are no message in the console. i.e, no click event fired.

What is problem?

Attached file test.html —

Ah, I attached the wrong test-case, sorry for the confusion.

The click events are prevented if the listener is added to the node itself, but not if they're attached to the document/window.

Attachment #9049425 - Attachment is obsolete: true

middle/right click isn't dispatched at all on elements.
That has been the behavior in Gecko... I think 15+years. But sure, we should change it, now that we have auxclick.

Yeah, I just really meant that my previous test-case was attaching to the node, which is clearly wrong for the intended effect for the reason you just cited.

John, would you like to help with this? :)

Flags: needinfo?(jdai)
Priority: -- → P2

Possibly the safest option would be to not set the flag in Document and nsGlobalWindowInner if
document (or GetExtandDoc in window case) isn't XUL document.
But need to still test if contextmenu works in devtools, since I think it uses xhtml, not xul.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)

John, would you like to help with this? :)

Sure, I'm going to take this bug. :) Thanks Olli points out direction.

Assignee: nobody → jdai
Status: NEW → ASSIGNED
Flags: needinfo?(jdai)

I'm going to fix this bug, I'll provide a patch in a day or two. Sorry for the delay, I was busy in other bugs.

(In reply to John Dai[:jdai] from comment #13)

Created attachment 9054902 [details]
Bug 1533630 - Shouldn't fire click events for middle and right clicks;

I do a manual test on my local machine, it seems to work fine. Since the try server tree is closed, I'll add a test case and put to try run before sending review request.

After applying this patch, it affects some of the tests:

  • testing/web-platform/tests/uievents/click/auxclick_event.html
  • browser/base/content/test/tabs/browser_paste_event_at_middle_click_on_link.js
  • dom/tests/mochitest/general/test_clipboard_events.html
  • editor/libeditor/tests/test_bug674770-1.html
  • editor/libeditor/tests/test_bug674770-2.html
  • editor/libeditor/tests/test_middle_click_paste.html
Type: enhancement → defect

Regression = wontfix??? It was working in firefox65. Have to wait until firefox68 for a fix? Don't complain about userbase attrition.

(In reply to lists from comment #18)

Regression = wontfix??? It was working in firefox65. Have to wait until firefox68 for a fix? Don't complain about userbase attrition.

Firefox 66 is released. We'd be very careful to manage the scope for a dot release, if any, for better risk and scalability management. This issue is unfortunate not a one we should take for that. We're working on a solution and uplifting to 67 could be a resolution to make it happen sooner. Thank you for caring about this and understanding. :)

(In reply to John Dai[:jdai] from comment #16)

More investigate each of test failures, it seems that we have multiple places which including click, auxclick, clipboard and editor need to consider as well. In summary, it contains 4 things need to address:

  • Our firefox architecture is base on click event, hence, our UI[1] and hacky[2] also base on click event, we should clean them up. Also, we need to avoid protential risk[3].
  • Click an editable link, it shouldn't open a new tab.
  • Middle click paste functionality fail to paste in contenteditable editor.
  • Middle click paste functionality in input element need to file an input event.

After applying this patch, it affects some of the tests:

  • testing/web-platform/tests/uievents/click/auxclick_event.html

This test will pass, we can remove the ini file.[4]

  • browser/base/content/test/tabs/browser_paste_event_at_middle_click_on_link.js

We expect that click on an editable link, it shouldn't open a new tab, but it fails. [5]

  • dom/tests/mochitest/general/test_clipboard_events.html

In this test[6], we listen to the click event when the user clicks the middle button, but now it's no longer filing click event when the user clicks the middle button. It can be fixed by revising test case.

  • editor/libeditor/tests/test_bug674770-1.html

The click operation on the link shouldn't open a new tab, but it fails. [7]

  • editor/libeditor/tests/test_bug674770-2.html

Middle click paste fail to paste test in contenteditable editor[8].

  • editor/libeditor/tests/test_middle_click_paste.html

Middle click paste is broken and input event is missing[9].

[1] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/browser/modules/ContentClick.jsm#42
[2] https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/editor/libeditor/EditorEventListener.cpp#622-635
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1461708#c34
[4] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/testing/web-platform/meta/uievents/click/auxclick_event.html.ini
[5] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/browser/base/content/test/tabs/browser_paste_event_at_middle_click_on_link.js#49
[6] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/dom/tests/mochitest/general/test_clipboard_events.html#927-939
[7] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/tests/test_bug674770-1.html#54
[8] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/tests/test_bug674770-2.html#203-208,228-233,255-262,309-317,343-349
[9] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/tests/test_middle_click_paste.html#234-240

So there are now some related patches also in bug 1379466

Depends on: 1379466

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Webcompat Priority: --- → ?

See bug 1547409. Migrating whiteboard priority tags to program flags.

Tom, given comment #21, can you re-test here? Thanks.

Flags: needinfo?(twisniewski)

Confirmed. My above test-case now no longer fires middle/right clicks.

That said, the New York Times page still isn't responding to middle-clicks on its Subscribe button for me. However, it's not because of something they are doing: I've confirmed that their scripts aren't getting any events, as desired. My system just isn't opening links in a new tab on middle-clicks for some reason, despite the relevant about:config flags being set, which is unrelated.

Flags: needinfo?(twisniewski)

I think we can close this now. Please file a new bug for that NYT issue if we think it's important.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

NYT's Subscribe button is now behaving similarly for me in Firefox and Chrome on middle/right clicks, so I don't think there's a remaining interop issue with it.

You need to log in before you can comment on or make changes to this bug.