52548 - Sidebar Links opening in Composer window.

Status: RESOLVED FIXED

(SeaMonkey :: Sidebar, defect, P1)

Description

[Joseph Elwell]

This is similar to bug 44797. In fact, the outcome is the same.
To reproduce:
1)Open Composer
2)Click on a link in the sidebar (i've got a comm build so I click on the AOL 
stock link, in the Stocks panel).
*watch as the link is opened in the composer window*

It doesn't matter if a browser window is open or not.
This is effecting IM (bugscape bug 2277) Chat windows also.
I've tested comm bits, winNT 2000090608 and linux 2000091312.

Comment 1

[shrirang khanzode]

wow...I see this..nominating for beta3. severity major

Comment 2

[Peter Trudelle]

nsbeta3+, p2 for M18

Comment 3

[Peter Trudelle]

PDT agrees, P2

Comment 4

[Dan M]

*** Bug 53271 has been marked as a duplicate of this bug. ***

Comment 5

[buster]

transferring comment from dup'd bug:  the editor session is lost when this 
occurs, meaning this is a *major* data loss bug.  Please give it a very high 
priority, it's as bad as a crasher.

Comment 6

[johng]

since this causes data loss in Composer, renominating to be P1

Comment 7

[clayton]

PDT agrees.  Data loss not acceptable.

Comment 8

[Peter Trudelle]

trudelle agrees, P1.  severity should really be critical

Comment 9

[Dan M]

  Has this ever worked, or is this a regression? I haven't completely traced into 
the sidebar code -- man, that's an education in JS -- but it looks to me that 
what's happening is, editor.xul identifies its <editor> element as the window's 
window._content frame. No doubt the sidebar code, which is built to live in a 
browser window, tries to load bookmarks in its parent's window._content frame.
  It's pretty much doing what you'd expect. I think that either the editor window 
needs to avoid identifying its content area as the browser content frame, or the 
sidebar code needs to behave differently when it finds itself in an editor 
window. Ideas?

Comment 10

[Peter Trudelle]

It isn't marked as a regression, so maybe it never worked.  Sidebar needs to 
live in a variety of windows, and possibly someday stand-alone.  I think that 
only one of these cases (browser) should load in the parent's content area.

Comment 11

[Joseph Elwell]

Now it's marked regression. ;)
This has worked in the past. See bug 44797, which is now verified Fixed - which
effected Composer, Mail/News and IM. Currently only IM and Composer are effected.

Comment 12

[Peter Trudelle]

mass-adding rtm keyword to all open nsbeta3+ xptoolkit bugs

Comment 13

[Michael La Guardia]

Marking nsbeta3- as we won't pull PR3 off the wire for this.  Bug already has
rtm nomination. Adding relnote keyword

Comment 14

[Dan M]

  The problem is, as I mentioned above, everything is behaving exactly as 
instructed. The bookmarks "open that link" code tries first to open that link in 
the current window, if there's a window._content frame. Since Composer contains 
such a thing, the link is opened in Composer, oy, oy.
  Netscape doesn't want a fix checked in just now, so I'll attach the diff here 
for posterity, in case I lose it before the tree loosens. The patch just forces 
the bookmarks code to look for a browser window, rather than settling on its 
current window. Itcauses a sidebar in Composer to open the link in an extant 
browser window, if any, or to open a new one, if not. In a browser window, it 
opens in the current window. Odd things would happen, however, if this code were 
for some reason triggered from a non-topmost browser window. As far as I know, 
it's triggered only by user interactions, so it should be safe.

Comment 15

[Dan M]

Attachment: patch reviewed and approved so far by ben, rjc

Comment 16

[Simon Fraser [no longer active]]

Why not use the openTopWin() function that lives in utilityOverlay.js? That does 
pretty much what your code is doing.

Comment 17

[Peter Trudelle]

marking rtm+.  Does it now use *any* browser window, or is it deterministic,
such as using the topmost browser window?  I think the behavior should be
predictable by the user.

Comment 18

[Phil Peterson]

PDT marking [rtm++]

Comment 19

[Phil Peterson]

Taking back the rtm++ since sfraser proposed a different patch. Please take off
need info when reviewers agree.

Comment 20

[Dan M]

Attachment: Simpler patch à la Simon. Reviewed by ben, rjc

Comment 21

[Peter Trudelle]

adding need info back. Brendan's rules say the reviewers need to put r=/a= in
the bug report.  That's what PDT looks for to grant ++ status.

Comment 22

[Simon Fraser [no longer active]]

r=sfraser. Who is SR for bookmarks?

Comment 23

[Dan M]

Oh, for the love of bureaucracy!!! It's Ben. Ben, one of the reviewers mentioned 
above. But I was probably lying about that. Better check with him.

Comment 24

[Joseph Elwell]

Neither patch will fix the original bug. As the problem is not specific to the
bookmark sidebar tab. Would a similar patch have to be checked in for *every*
sidebar tab?

Comment 25

[Dan M]

I find myself bothered by your assertion that the patch won't fix this bug, since 
I've just claimed that it will. Ben has also tried it and agrees.

Comment 26

[Joseph Elwell]

I just applied the patch successfully. Let me see if I can make this clear. I
presume your patch is to fix the fact that when you double click on the bookmark
while in Composer nothing happens?

The testcase on this bug (using a Mozilla sidebar tab rather than a Commercial
tab) is:
1) Launch Composer
2) Open the Tinderbox tab
3) Click on the link in the Tinderbox tab.
*watch as the Composer window is overwritten with tinderbox.mozilla.org...*

The problem appears to be these sidebar tabs have a TARGET="_content" in their
Base tag.

Here's an example:
http://sidebar.netscape.com/sidebar/wrapper.tmpl?service=stocks

Somewhere that target needs to be caught and redirected away from Composer and
to the Browser.

After applying the patch the test case for original bug is still broken. Which
is clicking on a *link* in the sidebar - not a bookmark.

Comment 27

[Joseph Elwell]

The where to load problem has come up on our team before. Vishy pointed me to
this piece of code:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3175

This is where the fix probably needs to be done. I am not familiar with this
code, cc'ing mscott as it appears he's done alot of work on that function.

Comment 28

[Dan M]

What do you know. Right you are. I misread the bug report and ended up fixing a 
different though related problem (doubleclick on a bookmark in Composer.)

Comment 29

[Simon Fraser [no longer active]]

A real fix for this should happen for editor embedding, I think, and what should 
probably happen is that editor has a special kind of docShell that prohibits 
calls to DoURILoad, and forces people to go through the editorShell to load URIs.

In the shorter term, we probably need to set a flag in the docShell to say that 
URI load should spawn a new window. I think even this is rather risky for RTM. 
Maybe a safer solution is to disable the sidebar in Composer?

Comment 30

[Joseph Elwell]

With this new insight does this bug need a new owner? or will work proceed?

Comment 31

[Simon Fraser [no longer active]]

We can't expect every sidebar author to not assume that loading in 
window._content is bad. Editor needs a different solution. I was hoping for some 
management outcry when I suggested removing sidebar from editor, but since there 
was none.....

Comment 32

[Simon Fraser [no longer active]]

Additional comments:

However, I just talked with hyatt/danm/waterson, and we came up
with the following issues:

1. We should not bastardize window._content to mean something different
   in mail/composer windows. Sidebar authors *should* be able to load
   in window._content if they really want to (think of XUL as a platform).

2. We need to create something that sidebar authors can migrate to
   and use in 6.0 and beyond, that means 'open in the current or a new
   browser window). This should be in place by RTM so that sidebar panels
   are compatible with 6, 6.5 etc.

   The suggestion is to create a new argument to window.open(),
   "_activecontent", that does what we need. Dispatch code would
   see this, and map it to "_blank" or "_current" as appropriate (depending
   on whether the current window was a browser window). This should be
   a small change localized in 2-3 places.

3. Pending the adoption of window.open(..., "_activecontent"...) by sidebar
   authors, we need to still go ahead and hide the sidebar or specific
   sidebar panels in composer/chat/mail as appropriate.

Comment 33

[Peter Trudelle]

So, is there any chance of a simple, low-risk fix for this in the next day or so?

Comment 34

[bijals (gone)]

Sfraser talked with Mscott and I believe there is a quick solution that he is 
looking into.  Simon - Any updates?

Comment 35

[Simon Fraser [no longer active]]

I have a patch in hand that fixes all but the bookmarks (which are a known issue, 
bug exists). Will attach patches shortly.

Comment 36

[Simon Fraser [no longer active]]

Taking.

Comment 37

[Simon Fraser [no longer active]]

Attachment: Diffs to nsEditorShell.h/.cpp to implement nsIURIContentListener

Comment 38

[Scott MacGregor]

Simon, your diffs look good to me. One question, why did you need to add:
+ #ifdef XP_MAC
+ #pragma mark -
+ #endif

to nsEditorShell?

(other than that question, sr=mscott)

Comment 39

[Simon Fraser [no longer active]]

+ #ifdef XP_MAC
+ #pragma mark -
+ #endif

just adds a handy separator to the function popup menu on Mac, which we use to 
separate interface implementations. It's harmless.

Comment 40

[Joseph Elwell]

tested this for AIM (also tested composer) and it works great. r=jelwell from
the IM team.

Comment 41

[Viswanath Ramachandran]

um u shd probably rtm+ this to get it on the PDT radar to ++. thx, Vishy

Comment 42

[Simon Fraser [no longer active]]

It's already [rtm need info]

Comment 43

[Peter Trudelle]

PDT queries for [rtm+], specifically filtering out the 'need info' bugs. It
would help to also send email to PDT asking for the double-plus.

Comment 44

[Kathleen :Brade]

fixing Status Whiteboard; I'll forward to pdt on Simon's behalf.

Comment 45

[Viswanath Ramachandran]

Simon pls confirm you tested on 2 platforms and that the patch is only the last
one i.e. 10/11/00. With that, r=vishy.

Comment 46

[Scott MacGregor]

sr=mscott

Comment 47

[Simon Fraser [no longer active]]

Tested on Windows, everything works fine. Only patch to be checked in is the 10/
11/00 patch.

Comment 48

[Phil Peterson]

rtm++ but (1) please don't clean up the QI on the branch (do that on the trunk
if you want) and (2) please check for null on the out params in the stub functions.

Comment 49

[Simon Fraser [no longer active]]

Attachment: The Final patch

Comment 50

[Simon Fraser [no longer active]]

Attached a revised patch, based on PDT recommendations. Used NS_ENSURE() macros 
to check for non-null arg pointers before dereferencing them, and remove the QI 
refactoring.

mscott, vishy, please re-review.

Comment 51

[Scott MacGregor]

sr=mscott on the latest patch with the PDt requested changes.

Comment 52

[Simon Fraser [no longer active]]

Fix checked into trunk (with refactored QI) and into branch (more conservative 
change).

Comment 53

[Viswanath Ramachandran]

r=vishy. 

Comment 54

[shrirang khanzode]

This is verifed on all branch builds, win/mac/linux 2000101608. Sidebar links no 
longer open in composer. Adding VTRUNK keyword for trunk verification.

Comment 55

[Asa Dotzler [:asa]]

Verified Fixed on trunk builds. Composner sidebar links load in browser window.
linux 101808 RedHat 6.2
win32 101804 NT 4
mac 101804 Mac OS9
Setting bug to Verified and removing vtrunk keyword

Comment 56

[[e-mail]]

I've just seen this in 1.4 and 1.5 trunk :-(

Steps to reproduce problem:
1. mozilla -edit
2. open tinderbox sidebar
3. click on SeaMonkey link

Actual results: overwrites currently edited document

Expected results: new browser window opens

Alternative steps:
1. mozilla -chat -compose
2. click on URL seen while chatting

Actual results: URL loads in compose window

Expected results: new browser window opens

Now the patch given patches the editor shell, which was removed.
Does this mean that this is a regression of removing the editor shell?

Comment 57

[[e-mail]]

Attachment: Converted for nsEditingSession and new nsIURIContentListener

Comment 58

[Kathleen :Brade]

Comment on attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

Does this impact midas use--do links elsewhere on the midas page still work?

Assuming so, r=brade

Comment 59

[[e-mail]]

The edit preferences link on the midas demo page itself still works.

Comment 60

[Johnny Stenback (:jst)]

Comment on attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

sr=jst

Comment 61

[[e-mail]]

Fix checked in.

Comment 62

[[e-mail]]

Comment on attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

Could be useful for 1.4.1

Comment 63

[Asa Dotzler [:asa]]

Comment on attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

We're trying to ship 1.4.1 quickly and might take this later on the branch but
not for 1.4.1 since our testing resources are limited and our time is short.

Comment 64

[Kathleen :Brade]

*** Bug 213478 has been marked as a duplicate of this bug. ***