199360 - mail slow to bring up a browser window when you click on a link.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[Ere Maijala (slow)]

Uh, we could start a debate about the use of frontmost browser window, but at
least make sure middle-clicking doesn't break.

Comment 2

[Samir Gehani]

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.

Comment 3

[Simon Fraser [no longer active]]

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.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: alternative patch

the alternative patch has issues, like the throbber in mail 3 pane still spins
on a link click.

Comment 6

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 7

[(not reading, please use [e-mail] instead)]

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.

Comment 8

[(not reading, please use [e-mail] instead)]

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.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 10

[(not reading, please use [e-mail] instead)]

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 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 119736 [details] [diff] [review]
patch

seeking review from brian

Comment 12

[Brian Ryner (not reading)]

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?

Comment 13

[(not reading, please use [e-mail] instead)]

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.

Comment 14

[(not reading, please use [e-mail] instead)]

here's the trick, we need to special case for all urls that don't require a
browser window (or special cause for those that do).

another one I missed, aim:goim?screename=saspitzer

javascript:alert('foo');

maybe some of these we want to block in mailnews, which is now possible.

I'll keep working on this.

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 120233 [details] [diff] [review]
updated patch

seeking r from bryner.

Comment 17

[Brian Ryner (not reading)]

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?

Comment 18

[(not reading, please use [e-mail] instead)]

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 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 120233 [details] [diff] [review]
updated patch

seeking re-review from bryner, per my last comment.

Comment 20

[Brian Ryner (not reading)]

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 21

[(not reading, please use [e-mail] instead)]

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.

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: version that plays nice with thunderbird.

Comment 23

[(not reading, please use [e-mail] instead)]

fixed.

thanks for bryner for the reviews, and to simon for pointing out this issue.

Comment 24

[(not reading, please use [e-mail] instead)]

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.

Comment 25

[[e-mail]]

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.

Comment 26

[(not reading, please use [e-mail] instead)]

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

Comment 27

[Ere Maijala (slow)]

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.

Comment 28

[esther]

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. 

Comment 29

[esther]

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.  

Comment 30

[(not reading, please use [e-mail] instead)]

this did cause that problem, I think.

on it.

Comment 31

[(not reading, please use [e-mail] instead)]

fix in hand, in bug #201652

Comment 32

[esther]

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

Comment 33

[(not reading, please use [e-mail] instead)]

I landed the fix for the issue raised in comments 27,28,29

Comment 34

[esther]

verified buidl 20030428 winxp, macosx and linux 

Comment 35

[Jesse Ruderman]

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

Comment 36

[Jesse Ruderman]

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

Comment 37

[Jesse Ruderman]

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

Comment 38

[http://tinymailto.com/oliversl]

I created bug #164710 3 months before this bug.
Sad it didn't get any attention :-(

Comment 39

[Matthias Versen [:Matti]]

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.