806136 - Fix code logic bug in addLinkClickCallback method (notification.xml)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Frank Wein [:mcsmurf]]

The addClickToPlayCallback method in notification.xml returns on a keydown event when the enter/return key is pressed:
linkNode.addEventListener("keydown",
                          function(aEvent) {
[...]
  if (aEvent.keyCode == aEvent.DOM_VK_RETURN)
    return;

This is wrong, the link should only be activated/followed when the enter key is pressed, not the other way round.

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Easy to review, also see http://hg.mozilla.org/mozilla-central/annotate/f9acc2e4d4e3/browser/base/content/browser-plugins.js#l101 about the code logic (this is where this code copied from, but the logic was not fixed when changing this part of the code).

Comment 2

[Frank Wein [:mcsmurf]]

To test this patch I recommend the following way: Disable any plugin via the Add-On manager (for example Flash or Adobe PDF) and then go to a page with an embedded Flash animation or a pdf doc (for example http://acroeng.adobe.com/test_files/embedded/embedded_weblink.html). Then select the "Manage plugins..." link (either with the tab key or hold the left mouse key over the link and then move away so that it doesn't click). As soon as you press any key then, it opens the Add-On manager (even enter works as this triggers the onClick handler in the same file :).

Comment 3

[Ian Neal]

Comment on attachment 675899 [details] [diff] [review]
Patch

>-                                        if (aEvent.keyCode == aEvent.DOM_VK_RETURN)
>+                                        if (!(aEvent.keyCode == aEvent.DOM_VK_RETURN))
Wouldn't using != be better?
r=me with that fixed.

Comment 4

[Frank Wein [:mcsmurf]]

Pushed with change from Comment 3: https://hg.mozilla.org/comm-central/rev/325af5a00c91

Though I wonder if that key handler there is actually required as the enter key also seems to fire a onclick event?

Comment 5

[Frank Wein [:mcsmurf]]

Comment on attachment 675899 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Unexpected behavior in the plugin click-to-play UI when a link in the UI is selected and the user presses any key on his keyboard (for example to switch to another tab, pressing the Windows key or whatever). A link in the click-to-play GUI appears when the user the CTP enabled ("click here to activate plugin"), when a plugin has been marked as vulnerable (then the plugin gets switched to CTP) or when a plugin has crashed ("Reload Page" link).
Testing completed (on m-c, etc.): Patch has been tested on comm-central
Risk to taking this patch (and alternatives if risky): Almost no risk.
String changes made by this patch: None

Comment 6

[Frank Wein [:mcsmurf]]

Hrm, I used the wrong check-in comment on -central and -aurora :) (wrong function name, see summary change)

Pushed to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/a2260c958d8d
Pushed to comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/e88cf81c4b65