Closed Bug 326737 Opened 15 years ago Closed 15 years ago

Shift key ignored when opening link in new tab from context menu

Categories

(SeaMonkey :: UI Design, defect)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.1alpha

People

(Reporter: csthomas, Assigned: csthomas)

Details

(Keywords: fixed-seamonkey1.1a)

Attachments

(1 file, 1 obsolete file)

The last parameter of http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsContextMenu.js#526 should be event.shiftKey instead of false.  Same for the openframeintab function below it.  This will require passing in an event to those functions, so http://lxr.mozilla.org/seamonkey/search?string=openlinkintab the command(s?) there will have to be fixed. Should still be pretty easy.

Might as well check to see who else calls openlinkintab without a useful third parameter.
Whiteboard: [good first bug]
Attached patch patch (obsolete) — Splinter Review
Attachment #211655 - Flags: superreview?(jag)
Attachment #211655 - Flags: review?(jag)
Comment on attachment 211655 [details] [diff] [review]
patch

+        openNewTabWith( this.linkURL(), true, aEvent && "shiftKey" in aEvent && aEvent.shiftKey );
+        openNewTabWith( this.target.ownerDocument.location.href, true, aEvent && "shiftKey" in aEvent && aEvent.shiftKey );

Since you've fixed both places that call this to always pass in |event| you don't need the |aEvent && "shiftKey" in aEvent && | part.
Attachment #211655 - Flags: superreview?(neil)
Attachment #211655 - Flags: superreview?(jag)
Attachment #211655 - Flags: review?(jag)
Attachment #211655 - Flags: review+
All three places (in two files).
Comment on attachment 211655 [details] [diff] [review]
patch

Or pass aEvent.shiftKey into openFooInTab.
Attachment #211655 - Flags: superreview?(neil) → superreview+
(which has the advantage of not breaking extensions - it will just default).
Whiteboard: [good first bug] → [good first bug] [cst: see neil's comment, fix, text, land]
Just in case we wanna handle other modifiers later on, if we pass in |event| now that won't be a problem then, but if we pass in a bool now then changing it to event later on will be harder. Just null check event and you should be good.
Attached patch patch v2Splinter Review
Attachment #211655 - Attachment is obsolete: true
Attachment #223185 - Flags: review?(jag)
Comment on attachment 223185 [details] [diff] [review]
patch v2

s/aShiftKey/reverseBackgroundPref/

Follow |openNewTabWith()|s lead in the name of the argument, and since the rest of this file doesn't use the a prefix, let's not here either.

With that fixed r=jag
Attachment #223185 - Flags: review?(jag) → review+
Attachment #223185 - Flags: approval-seamonkey1.1a?
Checked in on trunk with jag's nit.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug] [cst: see neil's comment, fix, text, land]
Comment on attachment 223185 [details] [diff] [review]
patch v2

a=me for SeaMonkey 1.1
Attachment #223185 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
You need to log in before you can comment on or make changes to this bug.