Closed Bug 278143 Opened 17 years ago Closed 17 years ago
"Force links that open new windows to open in tab" does not work for links inside mail body in GMail
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050112 Firefox/1.0+ This must have regressed very recently. Steps to reproduce: 1. Go to options and set "Force links that open new windows to open in tab" 2. Go to your GMail account 3. Open any email that contains any URL in its body 4. Click the link Actual result: The link opens in a new window Expected result: The link should open in a new tab
WFM in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20050111 Firefox/1.0+ Regressed between 2005-01-11-08-trunk and Now
Confirmed under Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20050112 Firefox/1.0+ -> New
Status: UNCONFIRMED → NEW
Ever confirmed: true
Confirmed using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20050112 Firefox/1.0+ (bangbang023)
I opened the page source of one of my emails that had a link in it, and this is the link tag looked like: <a onclick=\"return top.js.OpenExtLink(window,event,this)\" href=\"http://www.bengoodger.com\" target=_blank> The entire paragraph was inside some kind of JS function, which is why the quotes are all escaped. I don't know how that code is going to be handled when I submit this bug, it might not be readable. The part that is interesting is the onclick event. I don't have any idea what it does, but I bet that's the reason it opens a new window.
Caused by checkin for bug 103638. Reassigning.
Assignee: bugs → jst
window.open with no name specified is classified as an extant window, and so is never diverted into a new tab.
The problem is that nsGlobalWindow::WindowExists just passes the name to FindTarget, which finds the current docshell. I think the most straightforward fix is to make an empty aName into "_blank" or "_new" up front in nsGlobalWindow::OpenInternal. Alternately, we could change WindowExists to return false for an empty input.
(In reply to comment #7) >"_new" FYI _new is deprecated. I just wrote PRBool divertOpen = aName.IsEmpty() || WindowExists(aName);
Sure, that will work as a quick hack.... r+sr=bzbarsky
But a real fix is only triple the trivial complexity of the quick hack. I can't think of a useful exception to the rule that a non-name should never match any window.
Comment on attachment 172974 [details] [diff] [review] a non-name never matches This will make <a href="whatever"> open in a new window (since the target name will be empty). An empty target for links means "current docshell", whereas it means "new window" for window.open.
The patch doesn't seem to have that effect in my testing.
That's very odd. Do you have the preferences to force things into currently existing windows set or something? I just don't see how FindTarget() can find the target docshell with that patch...
Just out of interest, why has this bug been marked as blocking 1.0.1? It was my understanding that 1.0.1 was to be built off the old Aviary branch and given that this bug is a more recent regression, I wouldn't have thought it would have anything to do with 1.0.1
bug 103638 is apparently going on the branches, so fixes for regressions will also have to land. iirc, of course, this isn't going to be a visible pref, so impact is still minimal.
I think we shold take the code in comment 8, and I'll probably end up removing more of this stuff later in bug 277972....
The patch from comment 10 is more correct. But, whatever.
bz, danm's patch works because the doc shell has its own empty target check.
Comment on attachment 172974 [details] [diff] [review] a non-name never matches Ah, I see. So it does... it's pretty far away from the FindTarget() call. :( Dan, sorry about marking r- initially, but I can't in good conscience review a patch based on "well, it works, but I don't know why." I just did a bit of testing and the FindChildWithName change actually gives us compat with IE on <iframe name=""> and window.frames[""], though it breaks compat with Opera. For the FindItemWithNAme code, I think it would make more sense to just return null up front if the name is empty, no? Then we don't have to go through this whole function, call into our parent, etc, etc. With that change (and removal of the mName.IsEmpty() check that this patch adds), r+sr=bzbarsky.
For the record, I never said I didn't know why this patch works. I only said that I hadn't neglected to do a spot of testing before suggesting it. I had thought both would be assumed. It's checked in on the trunk.
Dan, I know you tested the patch. It's just that I couldn't tell _why_ your testing had the result that it did, and you didn't point out why the result was what it was, so... :(
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050208 Firefox/1.0+ WFM
17 years ago
Product: Firefox → Core
Comment on attachment 172974 [details] [diff] [review] a non-name never matches a=caillon (on behalf of drivers) for 1.0.1 and 1.7.6
Whiteboard: need landing → need trunk and 1.7 landing
Fixed on 1.0.1 and 1.7 branches.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Verified Fixed for Firefox only. Setting the "Force links..." pref now opens all the testcases in a new tab. Aviary 1.0.1 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050221 Firefox/1.0.1 Firefox Trunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050221 Firefox/1.0+ I'm not seeing the correct behavior with Mozilla 1.7.6. The prefs seem a bit different and I couldn't find anything in about:config that would allow all links to open in a new tab. I'm guessing this patch does not affect the Mozilla 1.7 branch. If anyone wants to test this further and log a new bug for any related problems, please do so. This is verified for Firefox only.
Status: RESOLVED → VERIFIED
(In reply to comment #26) >I'm not seeing the correct behavior with Mozilla 1.7.6. The aviary branch backend is forked from the 1.7 branch.
You need to log in before you can comment on or make changes to this bug.