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

Status: VERIFIED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Pavel Penaz]

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

Comment 1

[Pavel Penaz]

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

Comment 2

[Chris Blore]

Confirmed under Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6)
Gecko/20050112 Firefox/1.0+

-> New

Comment 3

[Mike Pesce (:By-Tor)]

Confirmed using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6)
Gecko/20050112 Firefox/1.0+ (bangbang023)

Comment 4

[Martin Meyer]

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.

Comment 5

[Dan M]

Caused by checkin for bug 103638. Reassigning.

Comment 6

[Dan M]

Attachment: testcase

window.open with no name specified is classified as an extant window, and so is
never diverted into a new tab.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[[e-mail]]

(In reply to comment #7)
>"_new"
FYI _new is deprecated.

I just wrote PRBool divertOpen = aName.IsEmpty() || WindowExists(aName);

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sure, that will work as a quick hack....  r+sr=bzbarsky

Comment 10

[Dan M]

Attachment: a non-name never matches

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 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Dan M]

The patch doesn't seem to have that effect in my testing.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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...

Comment 14

[Chris Blore]

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

Comment 15

[Mike Connor [:mconnor]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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....

Comment 17

[Dan M]

The patch from comment 10 is more correct. But, whatever.

Comment 18

[[e-mail]]

bz, danm's patch works because the doc shell has its own empty target check.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Dan M]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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... :(

Comment 22

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050208 Firefox/1.0+

WFM

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 24

[Johnny Stenback (:jst)]

Fixed on 1.0.1 and 1.7 branches.

Comment 25

[Johnny Stenback (:jst)]

Marking FIXED.

Comment 26

[Jay Patel [:jay]]

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.

Comment 27

[[e-mail]]

(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.