simon writes: "when reading bugmail, i click on the bug url in the mail because bugzilla is so slow, the mail throbber spins for a while, before dispatching to a browser window if, during that time, I load another msg, then the url dispatch is cancelled, so the bug never loads in a browser window it seems that it has to get data from the server before deciding what do to with it http should always just go to the frontmost browser window" I've seen this too, for a while, but failed to log a bug. it's a regression, I'll debug, but maybe darin has some insight.
Uh, we could start a debate about the use of frontmost browser window, but at least make sure middle-clicking doesn't break.
Mail triage team: Seth, Simon, can you give us a summary of the quantitative degradation in performance? Esther to follow up as well to help isolate the builds this first occured.
The issue here isn't really one of performance, it's one of usability (which could be construed as data loss). The problem is that I've requested an http URL load by clicking a link in the mail message. But if the server is slow to respond (like bugzilla), and I load another mail message, then that URL load is cancelled by the message load. That's the really annoying thing.
Created attachment 118606 [details] [diff] [review] alternative patch the alternative patch has issues, like the throbber in mail 3 pane still spins on a link click.
Mail triage team: nsbeta1+/adt3
actually, both these approaches have issues. think about some sending you a html page, and in that page, there is a html anchor? I'm now leaning towards debugging and finding a fix that makes it so sooner, before we get any data, we can somehow start the process of thinking we have data, so that the browser window opens right away. it appears that right now we wait until we get data back.
here's some background: when you left click on a link (in browser or message pane), we have code in nsGenericHTMLElement::HandleDOMEventForAnchors() that handles it. if you have control,meta,alt or shift held, we don't call TriggerLink(), and we don't create a OnLinkClick() event. instead, we let the click "go through" so that we can handle it in our js (to do a new tab, save link as, etc.) if you don't have those keys held, we call TriggerLink() which will generate an OnLinkClick event, which will get handled by the webshell / docshell. for browser, that's desired. we want it to replace the current content. so we we create the channel using the browser docshell's loadgroup, wait until we get some data (only then do we know the content type, and if we can handle it, or if we need to open something else.) but for mailnews, this causes some problems. when you click, the mail throbber starts up (since we are using the message pane docshell), and once we get some data and learn that the message pane doesn't want to handle it, we send it to a browser window (opening one, if we need to.) for a better user experience, if we do a simple left click on a url (that isn't mailto, (s)news, imap, mailbox), we really want to send to the topmost browser right away (or create one). bryner came up with a great suggestion on how to work around this. add a onclick listener to the message pane, to jump in early, and if I determine it's a simple left click on a link event, and not for a mailto,news,imap,mailbox url, send it to a browser window, and call preventDefault() on the event. otherwise, handled it as we used to. patch in hand.
Created attachment 119736 [details] [diff] [review] patch
here's what I tested: a) stand alone msg window, 3 pane (both layouts) b) simple left click of: mailto: relative anchors in imap, news and local messages ftp:// http:// c) ctrl click (new tab) shift click (should save) middle click (new tab) ctrl enter (when link has focus) enter (when link has focus) d) context menu stuff for links
Comment on attachment 119736 [details] [diff] [review] patch seeking review from brian
Comment on attachment 119736 [details] [diff] [review] patch >+ // if is a mail type url, do nothing, and let the normal code execute >+ var mailurls = /(^mailbox:|^imap:|^(s)?news:|^mailto:)/i; >+ if (href.search(mailurls) == 0) >+ return; There are some more protocols that mailnews handles, right? I see these in the tree also: nntp none movemail pop cid addbook Do we need to worry about these?
we only need to special case for the ones that appear as clickable links. nttp:// is one (good catch). I'll go look at the others.
Created attachment 120233 [details] [diff] [review] updated patch
Comment on attachment 120233 [details] [diff] [review] updated patch seeking r from bryner.
Comment on attachment 120233 [details] [diff] [review] updated patch I'm not sure I like mail having specific knowledge of which protocols the browser handles, particularly since new protocol handlers can be dropped in. Your previous approach made more sense to me, where mail knew what protocols it could handle itself. Can you explain what the problem was with that approach?
here's what the current code does: handle normally, and let mozilla figure it out (but might be slow) here's the two ways I could have fixed this: 1) if the scheme is something I *know* requires a browser window load in a browser window else handle normally, and let mozilla figure it out (but might be slow) or 2) if the scheme is something I know doesn't require a browser window handle normally, and let mozilla figure it out (but might be slow) else load in a browser window the patch is approach 1. the problem with approach 2 is, what if there is some new scheme (that we add support for), that should not be handled in a browser window? if I didn't fix the code, we'd open up a browser window. with approach 1, we'd handle it normally, like we used to, and not open a browser window.
Comment on attachment 120233 [details] [diff] [review] updated patch seeking re-review from bryner, per my last comment.
Comment on attachment 120233 [details] [diff] [review] updated patch Ok, I'm convinced. r or sr=me. Just out of curiousity, how would you handle this for the standalone mail case?
Comment on attachment 120233 [details] [diff] [review] updated patch for stand alone mail, we're adding a bool pref to mailnews.js (which the thunderbird guys will override) and I'll be checking it in messagePaneOnClick(), and if "mail.standalone" is true, I bail out early, and let the normal code do what it does. stand alone mail jumps in at the webshell, on the link click event, so there is no problem.
fixed. thanks for bryner for the reviews, and to simon for pointing out this issue.
turns out, stand alone mail has already forked mailWindow.js, so I undid that change. but the pref in mailnews.js will stay, for future use.
Reusing an existing browser window I have no issues with but I'm not entirely convinced that opening a new browser window is always correct; if the user really wants a new browser window then they can always use the context menu.
> Reusing an existing browser window I have no issues with but I'm not entirely > convinced that opening a new browser window is always correct; if the user > really wants a new browser window then they can always use the context menu. if I have a browser window open, I'll load the url in it. if not, I'll open a browser window. just like if you did a simple left click on the url. or did I misunderstand your comment?
Now this works pretty well otherwise, but the browser window isn't brought to the foreground. If I have Mail maximized, it looks almost like nothing happens when I click a link.
Ere's comment is still happening with 20030423 build (there is a bug similiar to this only the browser window is minimized bug 201652) I will investigate to see if this bug fixed caused the link not bringing the browser window up or to the front and then update this bug or verify it.
Reopening this bug, I think it caused bug 201652. This bug was fixed on 4-16, 4-15 builds bring up the browser window (slowly) when clicking on a link in mail whether it's minimized or maximized. 4-18 build does not bring up a browser window when clicking on a link in mail whether it's minimized or maximized after it loads the link location. If the fixt to this bug did not cause this new problem, resolve this again and I'll push on bug 201652.
this did cause that problem, I think. on it.
fix in hand, in bug #201652
*** Bug 201652 has been marked as a duplicate of this bug. ***
I landed the fix for the issue raised in comments 27,28,29
verified buidl 20030428 winxp, macosx and linux
*** Bug 134421 has been marked as a duplicate of this bug. ***
*** Bug 157527 has been marked as a duplicate of this bug. ***
*** Bug 130671 has been marked as a duplicate of this bug. ***
I created bug #164710 3 months before this bug. Sad it didn't get any attention :-(
Oliver: It can happen with the flood of incoming bugs. Just look how many dupes we have for this one and we only have limited triager and developer resources.