Closed Bug 738786 Opened 12 years ago Closed 10 years ago

Middle clicking a tab does nothing if contentLoadURL is true but autoScroll is also true.

Categories

(SeaMonkey :: Tabbed Browser, defect)

SeaMonkey 2.9 Branch
x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.26

People

(Reporter: therubex, Assigned: ewong)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120323 Firefox/13.0a2 SeaMonkey/2.10a2
Build ID: 20120323013002

Steps to reproduce:

 
Per Bug 284379, with the preference, middlemouse.contentLoadURL, enabled (set to true), middle clicking a tab should still close tab.
 


Actual results:

 
Tab is not closed.
No action occurs.
 


Expected results:

 
Tab should have been closed.
 
 
With the pref set, when center clicking on a tab:

SeaMonkey 2.0.14, enters the contents of your clipboard into the Location Bar.
SeaMonkey 2.1, does nothing.  And continues that way up through Aurora (at least).
Blocks: FF2SM
Depends on: 284379
Regression from Bug 107147 ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
On Linux, I had to disable middlemouse.contentLoadURL as the behavior of pasting whatever happens to be on top of the clipboard was loaded whenever trying to close the tab with a middle mouse click (it's set "true" by default on Linux). Thus, I'll have to agree with the critics in bug 107147 saying that the solution there isn't optimal (and I'm wondering what the motivation is for making it the default on Linux but not Windows, despite the fact that middle-click into a /content/ area usually means "paste" on UNIX-like systems, but that's controlled by middlemouse.paste and shouldn't affect URL pasting).

On Windows, both 2.17.1 and trunk builds still exhibit the behavior as stated here, with  middlemouse.contentLoadURL = true, nothing is happening when middle-clicking a tab, and no messages in the Error Console either.
BTW: Porting bug 284379 should take care of closing the tab on Windows as well, it just removes the dependency on middlemouse.contentLoadURL, but won't solve the issue of pasting not working on Windows.
Blocks: 96972
Attached patch Proposed patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8342343 - Flags: review?(neil)
Comment on attachment 8342343 [details] [diff] [review]
Proposed patch (v1)

The actual problem here is that content load url is also disabled if autoscroll is enabled. This means that if you enable both autoscroll and content load url then middle-clicking tabs does nothing, when it should actually remove the tab.

The correct fix is therefore to enhance the preference check to include the autoscroll preference, and not return early unless autoscroll is also disabled.

(If you only have one pref on then everything works as expected of course.)
Attachment #8342343 - Flags: review?(neil) → review-
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8342343 - Attachment is obsolete: true
Attachment #8344614 - Flags: review?(neil)
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8344614 - Attachment is obsolete: true
Attachment #8344614 - Flags: review?(neil)
Attachment #8344616 - Flags: review?(neil)
Comment on attachment 8344616 [details] [diff] [review]
proposed patch (v2)

Sorry I wasn't watching IRC in time to stop you confusing locked with disabled.

By the way, attachment 8344614 [details] [diff] [review] was slightly less incorrect in that it did at least use the correct Boolean operator (but some parentheses might have been advisable for readability).
Attachment #8344616 - Flags: review?(neil) → review-
Attached patch proposed patch (v3) (obsolete) — Splinter Review
Attachment #8344616 - Attachment is obsolete: true
Attachment #8345264 - Flags: review?(neil)
Comment on attachment 8345264 [details] [diff] [review]
proposed patch (v3)

>+                !this.mPrefs.getBoolPref("general.autoscroll"))
Annoyingly the pref name uses an uppercase S. r=me with that fixed.
Attachment #8345264 - Flags: review?(neil) → review+
Summary: Port Bug 284379 - Enabling middlemouse.contentLoadURL and middle clicking a tab should still close tab → Middle clicking a tab does nothing if contentLoadURL is true but autoScroll is also true.
Comment on attachment 8345264 [details] [diff] [review]
proposed patch (v3)

Oh, and an extra set of ()s would help to reduce confusion i.e.
if (... || .. || (... && ...))
Attachment #8345264 - Attachment is obsolete: true
Attachment #8346525 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/985c8c6b102b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.26
No longer blocks: FF2SM
Comment on attachment 8346525 [details] [diff] [review]
proposed patch (v4)

>+                (this.mPrefs.getBoolPref("middlemouse.contentLoadURL") &&
>+                !this.mPrefs.getBoolPref("general.autoScroll")))
Strictly speaking the ! is inside the () so should have had an extra space indent.
(But it does look nicer this way!)
You need to log in before you can comment on or make changes to this bug.