Last Comment Bug 806136 - Fix code logic bug in addLinkClickCallback method (notification.xml)
: Fix code logic bug in addLinkClickCallback method (notification.xml)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.16
Assigned To: Frank Wein [:mcsmurf]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-27 14:35 PDT by Frank Wein [:mcsmurf]
Modified: 2012-11-11 05:21 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Patch (805 bytes, patch)
2012-10-27 15:04 PDT, Frank Wein [:mcsmurf]
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Frank Wein [:mcsmurf] 2012-10-27 14:35:31 PDT
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] 2012-10-27 15:04:22 PDT
Created attachment 675899 [details] [diff] [review]
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] 2012-10-27 15:12:29 PDT
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 2012-11-03 09:20:46 PDT
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] 2012-11-08 17:22:23 PST
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] 2012-11-10 04:11:59 PST
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] 2012-11-11 05:18:57 PST
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

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