Sidebar Links opening in Composer window.

RESOLVED FIXED in M18

Status

SeaMonkey
Sidebar
P1
major
RESOLVED FIXED
18 years ago
13 years ago

People

(Reporter: Joseph Elwell, Assigned: Simon Fraser)

Tracking

({dataloss, regression, relnote})

Trunk
dataloss, regression, relnote

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][PDTP1][rtm++] patch)

Attachments

(5 attachments)

(Reporter)

Description

18 years ago
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

18 years ago
wow...I see this..nominating for beta3. severity major
Severity: normal → major
Keywords: nsbeta3

Comment 2

18 years ago
nsbeta3+, p2 for M18
Priority: P3 → P2
Whiteboard: [nsbeta3+]
Target Milestone: --- → M18

Comment 3

18 years ago
PDT agrees, P2
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP2]

Comment 4

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

Comment 5

18 years ago
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.

Updated

18 years ago
Keywords: dataloss
Hardware: PC → All

Comment 6

18 years ago
since this causes data loss in Composer, renominating to be P1
Priority: P2 → P1
Whiteboard: [nsbeta3+][PDTP2] → [nsbeta3+]

Comment 7

18 years ago
PDT agrees.  Data loss not acceptable.
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP1]

Comment 8

18 years ago
trudelle agrees, P1.  severity should really be critical

Comment 9

18 years ago
  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

18 years ago
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.
(Reporter)

Comment 11

18 years ago
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.
Keywords: regression

Comment 12

18 years ago
mass-adding rtm keyword to all open nsbeta3+ xptoolkit bugs
Keywords: rtm

Comment 13

18 years ago
Marking nsbeta3- as we won't pull PR3 off the wire for this.  Bug already has
rtm nomination. Adding relnote keyword
Keywords: relnote
Whiteboard: [nsbeta3+][PDTP1] → [nsbeta3-][PDTP1]

Comment 14

18 years ago
  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.
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3-][PDTP1] → [nsbeta3-][PDTP1] fix in hand

Comment 15

18 years ago
Created attachment 15637 [details] [diff] [review]
patch reviewed and approved so far by ben, rjc
(Assignee)

Comment 16

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

Comment 17

18 years ago
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.
Whiteboard: [nsbeta3-][PDTP1] fix in hand → [nsbeta3-][PDTP1][rtm+] fix in hand

Comment 18

18 years ago
PDT marking [rtm++]
Whiteboard: [nsbeta3-][PDTP1][rtm+] fix in hand → [nsbeta3-][PDTP1][rtm++] fix in hand

Comment 19

18 years ago
Taking back the rtm++ since sfraser proposed a different patch. Please take off
need info when reviewers agree.
Whiteboard: [nsbeta3-][PDTP1][rtm++] fix in hand → [nsbeta3-][PDTP1][rtm need info] fix in hand

Comment 20

18 years ago
Created attachment 16008 [details] [diff] [review]
Simpler patch à la Simon. Reviewed by ben, rjc

Updated

18 years ago
Whiteboard: [nsbeta3-][PDTP1][rtm need info] fix in hand → [nsbeta3-][PDTP1][rtm+] reviewed patch attached 10/2

Comment 21

18 years ago
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.
Whiteboard: [nsbeta3-][PDTP1][rtm+] reviewed patch attached 10/2 → [nsbeta3-][PDTP1][rtm need info] reviewed patch attached 10/2
(Assignee)

Comment 22

18 years ago
r=sfraser. Who is SR for bookmarks?

Comment 23

18 years ago
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.
(Reporter)

Comment 24

18 years ago
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

18 years ago
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.
(Reporter)

Comment 26

18 years ago
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.
(Reporter)

Comment 27

18 years ago
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

18 years ago
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.)
(Assignee)

Comment 29

18 years ago
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?
(Reporter)

Comment 30

18 years ago
With this new insight does this bug need a new owner? or will work proceed?
(Assignee)

Comment 31

18 years ago
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.....

Updated

18 years ago
Whiteboard: [nsbeta3-][PDTP1][rtm need info] reviewed patch attached 10/2 → [nsbeta3-][PDTP1][rtm need info] experimenting...
(Assignee)

Comment 32

18 years ago
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

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

Comment 34

18 years ago
Sfraser talked with Mscott and I believe there is a quick solution that he is 
looking into.  Simon - Any updates?
(Assignee)

Comment 35

18 years ago
I have a patch in hand that fixes all but the bookmarks (which are a known issue, 
bug exists). Will attach patches shortly.
Whiteboard: [nsbeta3-][PDTP1][rtm need info] experimenting... → [nsbeta3-][PDTP1][rtm need info] patch
(Assignee)

Comment 36

18 years ago
Taking.
Assignee: danm → sfraser
Status: ASSIGNED → NEW
(Assignee)

Comment 37

18 years ago
Created attachment 16803 [details] [diff] [review]
Diffs to nsEditorShell.h/.cpp to implement nsIURIContentListener

Comment 38

18 years ago
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)
(Assignee)

Comment 39

18 years ago
+ #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.
(Reporter)

Comment 40

18 years ago
tested this for AIM (also tested composer) and it works great. r=jelwell from
the IM team.
um u shd probably rtm+ this to get it on the PDT radar to ++. thx, Vishy
(Assignee)

Comment 42

18 years ago
It's already [rtm need info]
Status: NEW → ASSIGNED

Comment 43

18 years ago
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

18 years ago
fixing Status Whiteboard; I'll forward to pdt on Simon's behalf.
Whiteboard: [nsbeta3-][PDTP1][rtm need info] patch → [nsbeta3-][PDTP1][rtm+] patch
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

18 years ago
sr=mscott
(Assignee)

Comment 47

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

Comment 48

18 years ago
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.
Whiteboard: [nsbeta3-][PDTP1][rtm+] patch → [nsbeta3-][PDTP1][rtm++] patch
(Assignee)

Comment 49

18 years ago
Created attachment 17086 [details] [diff] [review]
The Final patch
(Assignee)

Comment 50

18 years ago
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

18 years ago
sr=mscott on the latest patch with the PDt requested changes.
(Assignee)

Comment 52

18 years ago
Fix checked into trunk (with refactored QI) and into branch (more conservative 
change).
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
r=vishy. 

Comment 54

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

Comment 55

18 years ago
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
Status: RESOLVED → VERIFIED
Keywords: vtrunk

Comment 56

15 years ago
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?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 57

15 years ago
Created attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

Updated

15 years ago
Attachment #129216 - Flags: superreview?(peterv)
Attachment #129216 - Flags: review?(brade)

Comment 58

15 years ago
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
Attachment #129216 - Flags: review?(brade) → review+

Comment 59

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

Updated

15 years ago
Attachment #129216 - Flags: superreview?(peterv) → superreview?(jst)
Comment on attachment 129216 [details] [diff] [review]
Converted for nsEditingSession and new nsIURIContentListener

sr=jst
Attachment #129216 - Flags: superreview?(jst) → superreview+

Comment 61

15 years ago
Fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago15 years ago
Resolution: --- → FIXED

Comment 62

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

Could be useful for 1.4.1
Attachment #129216 - Flags: approval1.4.x?

Comment 63

15 years ago
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.
Attachment #129216 - Flags: approval1.4.x? → approval1.4.x-

Comment 64

15 years ago
*** Bug 213478 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.