Closed Bug 278143 Opened 16 years ago Closed 16 years ago

"Force links that open new windows to open in tab" does not work for links inside mail body in GMail

Categories

(SeaMonkey :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: pavel.penaz, Assigned: jst)

References

Details

(Keywords: fixed-aviary1.0.1, fixed1.7.6, regression, Whiteboard: need trunk and 1.7 landing)

Attachments

(2 files)

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
Keywords: regression
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.
Flags: blocking-aviary1.1?
Caused by checkin for bug 103638. Reassigning.
Assignee: bugs → jst
Attached file testcase
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.
Attachment #172974 - Flags: review-
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
Blocks: 103638
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.
Attachment #172974 - Flags: superreview+
Attachment #172974 - Flags: review-
Attachment #172974 - Flags: review+
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
Flags: review+
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
Attachment #172974 - Flags: approval1.7.6+
Attachment #172974 - Flags: approval-aviary1.0.1+
Whiteboard: need landing
Whiteboard: need landing → need trunk and 1.7 landing
Fixed on 1.0.1 and 1.7 branches.
Keywords: fixed1.7.6
Marking FIXED.
Status: NEW → RESOLVED
Closed: 16 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.
Flags: blocking-aviary1.1?
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.