1739929 - Cmd-clicking a link in Gmail stops working because it relies on untrusted page-generated click events

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Andrew Overholt [:overholt]]

Some time in the past week-ish I've noticed that I can no longer Cmd-click on a link in an email in Gmail and have it open a new tab. This used to work but Chris Peterson tried to reproduce in older versions of Firefox and it doesn't ... maybe they changed something on their side? Or maybe we changed how we're sending key events?

STR

1. open an email in Gmail. The email body must have at least one link in it.
2. Hold command (or Control on Windows/Linux) and click on the link in the email

Expected (and Chris Peterson confirms works in Chrome and Safari)

• link opened in a new tab

Actual

• nothing (and nothing in the console)

Comment 1

[Andrew Overholt [:overholt]]

Hsin-Yi, do you know if something's changed with our keyboard event handling/sending in the recent past? Or maybe Gmail is doing something differently now, but it's not working with Firefox?

Comment 2

[Chris Peterson [:cpeterson]]

Masayuki, Cmd+clicking a link in an email in Gmail no longer opens the link. It used to open a link in a new tab, just like it still does in Chrome and Safari. (I have only tested macOS.)

This does not seem to be a Firefox regression because I can reproduce this problem in Firefox Nightly 72 (November 2019).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I cannot reproduce this bug. Something new code is now rolling out?

Comment 4

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

(why key events are related here? It is ctrl+mouse click after all, no?)

I can't reproduce on linux. ctrl+click works here. Have they rolled out something in US+Canada but not yet elsewhere?

And ctrl+click is handled in Firefox frontend code.

Comment 5

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #4)

(why key events are related here? It is ctrl+mouse click after all, no?)

I can't reproduce on linux. ctrl+click works here. Have they rolled out something in US+Canada but not yet elsewhere?

And ctrl+click is handled in Firefox frontend code.

Yeah, but gmail handles this stuff themselves, x-ref e.g. bug 1429831. I expect they're calling preventDefault or something. :-\

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Olli Pettay [:smaug] from comment #4)

(why key events are related here? It is ctrl+mouse click after all, no?)

I can't reproduce on linux. ctrl+click works here. Have they rolled out something in US+Canada but not yet elsewhere?

Hey Ksenia,
Would you please help reproduce this issue? It sounded like this issue is likely due to gmail new code.

And ctrl+click is handled in Firefox frontend code.

Comment 7

[Andrew Overholt [:overholt]]

(FWIW I can middle-click but not Cmd+click)

Comment 8

[Chris Peterson [:cpeterson]]

This seems to be a macOS-specific issue. I can reproduce this bug with Cmd+click on macOS, but not with Ctrl+click on Windows.

(I am in the US.)

Comment 9

[Ksenia Berezina [:ksenia]]

I can reproduce this as well on macOS, will try to debug it

Comment 10

[Ksenia Berezina [:ksenia]]

Attachment: gmail.html

I've attached a reduced test case. In the onclick handler a new <a> element is created and a new event is dispatched on this element with createEvent, initMouseEvent, dispatchEvent. The original event has preventDefault(). Looks like this way of dispatching events in combination with Command key is not working in Firefox, but works in Chrome.

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Masayuki, can you follow up from what Ksenia found in comment 10? Thank you!

Comment 12

[Alice0775 White]

Sorry, this is wrong.

Comment 13

[Alice0775 White]

Sorry, this is wrong.

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Ksenia Berezina [:ksenia] from comment #10)

Created attachment 9249866 [details]
gmail.html

I've attached a reduced test case. In the onclick handler a new <a> element is created and a new event is dispatched on this element with createEvent, initMouseEvent, dispatchEvent. The original event has preventDefault(). Looks like this way of dispatching events in combination with Command key is not working in Firefox, but works in Chrome.

It is because JSWindowActor doesn't listen to the untrusted event by default and ClickHandlerChild doesn't handle the untrusted event, either, see https://searchfox.org/mozilla-central/rev/a48e21143960b383004afa9ff9411c5cf6d5a958/browser/actors/ClickHandlerChild.jsm#35. We behave the same on all platforms (Window, Mac, and Linux). We could "fix" this to make command+click work in Gmail by allowing ClickHandlerChild to handle the untrusted click event.

However, command+click in Gmail doesn't work on Mac only, I wonder if Gmail dispatches a new click event for Mac only, not sure if it is a bug of Gmail or they behave differently on Mac for some purpose. Hi Ksenia, could you help to check this with Gmail? Thanks!

Comment 15

[Ksenia Berezina [:ksenia]]

That's a good point, thanks, Edgar. I've just checked it on Windows and there is indeed a different path, where a new click event is not created and the original one is being used instead.

There is a function that is responsible for that with a condition if (!(b = _.gs && (a.ctrlKey || a.shiftKey))). Basically, if either of these keys are pressed, the function returns early with true. On Mac it's false because we're pressing Command key and not Control key (a.ctrlKey is false).

    s8c.prototype.Ca = function (a) {
      var b;
      if (!(b = _.gs && (a.ctrlKey || a.shiftKey))) a: {
        for (a = a.target; a; ) {
          if (a.hasAttribute(_.kj) && 'gb' == a.getAttribute(_.kj)) {
            b = !0;
            break a
          }
          a = _.Bq(a)
        }
        b = !1
      }
      return b
    };

Perhaps a.metaKey can be added to this condition to account for the Command key on Mac. But yeah, it's unclear why it's done this way. I'll reach out to Google on our mailing list to see if they can help us

Comment 16

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

Attachment: untrusted_click.html

Click events are special, also untrusted events should trigger links. And I think that should apply to ctrl+click too. And since ctrl+click (or the similar thing on MacOS) is handled in the frontend code, this is a frontend bug ;)

When fixing this, one needs to ensure that web pages can't use this mechanism to bypass popup blocking.

Comment 17

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

Ok, so this is a frontend bug. Untrusted click event is supposed to trigger links.

(We could consider to move ctrl+click handling to platform code. I think it lives in the frontend/JS mostly because of historical reasons.)

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Olli Pettay [:smaug] from comment #17)

Ok, so this is a frontend bug. Untrusted click event is supposed to trigger links.

(We could consider to move ctrl+click handling to platform code. I think it lives in the frontend/JS mostly because of historical reasons.)

Gijs, this is a recent Gmail regression (in Gmail's code) that, IIUC, should be fixed in Firefox's frontend code. I'm afraid this bug will get lost in the Firefox::General Bugzilla component, but I don't know what other component is more appropriate.

Comment 19

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #16)

Created attachment 9250306 [details]
untrusted_click.html

Click events are special, also untrusted events should trigger links. And I think that should apply to ctrl+click too. And since ctrl+click (or the similar thing on MacOS) is handled in the frontend code, this is a frontend bug ;)

When fixing this, one needs to ensure that web pages can't use this mechanism to bypass popup blocking.

Well, right, that's why the check is there. How would we ensure this? Last I checked, the mechanics of the popup blocker are not exposed to our frontend code, which makes this a core/DOM bug ;-)

Would it be enough to check if the document has user activation, and call consumeUserGestureActivation if we are going to act on the click? I think that's the best the frontend could do at the moment, but perhaps there's more exposed DOM stuff I'm not aware of...

I don't think there's a better component for this at the moment, unfortunately. The click handler child code itself also is listed under Fx::General. This bug just needs an assignee, but for that to happen we also need a viable approach to fixing it.

(In reply to Olli Pettay [:smaug] from comment #17)

(We could consider to move ctrl+click handling to platform code. I think it lives in the frontend/JS mostly because of historical reasons.)

I agree and I think this would be a good idea for other reasons (e.g. Fission memory use), though it's not entirely trivial because some of the code is shared with e.g. autoscroll code and context menu code, so we might need to add some webidl/xpidl exposure of either shared C++ or shared JS code, or snowball this into moving all that code into C++, which definitely wouldn't be upliftable if we want a fix to go out with 95...

Comment 20

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

I guess we'd need something like https://searchfox.org/mozilla-central/rev/3c8a7970944daaf917b547dffc0790bcd37cadc1/dom/base/nsGlobalWindowOuter.cpp#7104-7105,7107-7108 exposed to JS.
edgar might have ideas here.

Comment 21

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to :Gijs (he/him) from comment #19)

Would it be enough to check if the document has user activation, and call consumeUserGestureActivation if we are going to act on the click?

If we only want to prevent multiple popup being opended from ctrl+click, consuming the user gesture would be enough.
If we want to show the popup blocked warning when blocked or allow the website in popup exception list to open a popup from ctrl+click unconditional, we would need to expose API to JS.

I tested Chrome a bit, they consume the user activation, but if there is no valid user activation, the click is just a noop (no warning shown).
So it is probably fine to behave the same as Chrome, i.e. just consume the user activation?

Comment 22

[:Gijs (he/him)]

Attachment: Bug 1739929 - allow untrusted click/auxclick events to open one link per transient user gesture, r?edgar

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fb0990124cf
allow untrusted click/auxclick events to open one link per transient user gesture, r=edgar

Comment 24

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/0fb0990124cf

Comment 25

[Chris Peterson [:cpeterson]]

Verified fixed in Nightly 96.0a1 build ID 20211123215113.

Comment 26

[:Gijs (he/him)]

Comment on attachment 9252032 [details]
Bug 1739929 - allow untrusted click/auxclick events to open one link per transient user gesture, r?edgar

Beta/Release Uplift Approval Request

• User impact if declined: Cmd-clicking links in gmail is broken
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See earlier comments
• List of other uplifts needed: n/a
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): We're changing how we respond to untrusted (ie webpage-generated) click events. It's possible (though unlikely) that this will cause unexpected results for pages that relied on the previous behaviour somehow. However, there isn't really a less risky way to address the issue, gmail is very high-usage so it seems important to fix, and it ultimately puts our behaviour more in line with that of other browsers.
• String changes made/needed: none

Comment 27

[:Gijs (he/him)]

Comment on attachment 9252032 [details]
Bug 1739929 - allow untrusted click/auxclick events to open one link per transient user gesture, r?edgar

Clearing beta uplift request while Edgar looks at bug 1742801.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Comment 29

[:Gijs (he/him)]

I discussed with Pascal, the release manager, and we think at the moment the risk/reward balance is such that we shouldn't uplift this. We're out of betas (only release candidates left) and this already caused 1 regression, and has the possibility to do so for non-macOS, non-gmail users as well. If we end up doing a dot-release later we could reconsider, once the patches have had more baking and we're more confident they didn't break anything else.

Comment 30

[:Gijs (he/him)]

Release Note Request for Firefox 95
[Why is this notable]: A gmail change broke this for macOS users, it's likely to be noticed by users and we're not shipping the fix in 95 which means it's likely not shipping this year.
[Affects Firefox for Android]: No
[Suggested wording]: Known issue: command-clicking links in Gmail does not open a new tab. Workaround: click links in Gmail without pressing command, which will still open a new tab. [Optional addition: A fix is being tested in Firefox beta 96. -- w/ a link to the beta page]

Comment 31

[Chris Peterson [:cpeterson]]

Should we uplift this Gmail fix to ESR 91? Maybe after waiting a release cycle for the fix to successfully ship on the Release channel?

Comment 32

[:Gijs (he/him)]

[Tracking Requested - why for this release]:

(In reply to Chris Peterson [:cpeterson] from comment #31)

Should we uplift this Gmail fix to ESR 91? Maybe after waiting a release cycle for the fix to successfully ship on the Release channel?

I think so, but it'll need to come together with the fix in bug 1742801 and yes, it'll need some more baking first.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Added to the Fx96 relnotes as a fixed item as well.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Is this looking good for an ESR approval request or do we want to let it bake for another cycle?

Comment 35

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)

Is this looking good for an ESR approval request or do we want to let it bake for another cycle?

If you want to take it then it'd be this + bug 1742801, and probably bug 1745720 unless we want to take the associated perf regressions.

Given that last bug is only just landed on 98, I don't know how you wanna play it. Uplift the other 2 bugs now and live with the perf regression for 1 esr round? Uplift everything 1 release from now? wontfix for esr? Something else?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Yeah, I guess the perf bug is what was giving me pause. Given the nature of the regression, I'm kinda-sorta inclined to live with it on ESR rather than backporting that fix though?

Comment 37

[:Gijs (he/him)]

Comment on attachment 9252032 [details]
Bug 1739929 - allow untrusted click/auxclick events to open one link per transient user gesture, r?edgar

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes cmd-click / middle click on links in gmail
• User impact if declined: See above
• Fix Landed on Version: 96
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Has had a bunch of bake time, and we've already identified the one regression it's caused (bug 1742801).

Comment 38

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue with the TC's provided in the comments 10 and 16 with 96.0a1 (2021-11-07) on Ubuntu 20.04 and MacOS 12. I can confirm that the issue is fixed with firefox 98.0a1, 96.0.2 and 97.0b7 on MacOS 12, Ubuntu 20.04 and Windows 10. Leaving the qe-verify+ and the qa-triaged tags until the ESR request has been answered.

Have a nice day!

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9252032 [details]
Bug 1739929 - allow untrusted click/auxclick events to open one link per transient user gesture, r?edgar

Approved for 91.6esr.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/fac80dc79ea2

Comment 41

[Negritas Sergiu, Desktop QA]

Hello! I can confirm that the issue is fixed with firefox 91.6.0 esr provided in comment 40 on Ubuntu 20 and MacOS 12, updating the flags accordingly.

Thank you!