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

VERIFIED FIXED in mozilla1.4beta

Status

SeaMonkey
MailNews: Message Display
VERIFIED FIXED
15 years ago
12 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.4beta
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Keywords: nsbeta1
Target Milestone: --- → mozilla1.4beta

Comment 1

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

15 years ago
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.
Whiteboard: [need info]

Comment 3

15 years ago
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 118600 [details] [diff] [review]
patch
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.

Comment 6

15 years ago
Mail triage team: nsbeta1+/adt3
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [need info] → [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.
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Hardware: PC → All
Created attachment 119736 [details] [diff] [review]
patch
Attachment #118600 - Attachment is obsolete: true
Attachment #118606 - Attachment is obsolete: true
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
Attachment #119736 - Flags: review?(bryner)
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.
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.
Created attachment 120233 [details] [diff] [review]
updated patch
Attachment #119736 - Attachment is obsolete: true
Attachment #119736 - Flags: review?(bryner)
Comment on attachment 120233 [details] [diff] [review]
updated patch

seeking r from bryner.
Attachment #120233 - Flags: review?
Attachment #120233 - Flags: review? → review?(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?
Attachment #120233 - Flags: review?(bryner) → review-
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.
Attachment #120233 - Flags: review- → review?(bryner)
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?
Attachment #120233 - Flags: review?(bryner) → review+
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.
Created attachment 120739 [details] [diff] [review]
version that plays nice with thunderbird.
fixed.

thanks for bryner for the reviews, and to simon for pointing out this issue.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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

15 years ago
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?

Comment 27

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

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

15 years ago
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.  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this did cause that problem, I think.

on it.
fix in hand, in bug #201652
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 32

15 years ago
*** Bug 201652 has been marked as a duplicate of this bug. ***
I landed the fix for the issue raised in comments 27,28,29

Comment 34

15 years ago
verified buidl 20030428 winxp, macosx and linux 
Status: RESOLVED → VERIFIED
Attachment #120739 - Attachment is obsolete: true

Comment 35

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

Comment 36

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

Comment 37

15 years ago
*** 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.

Product: Browser → Seamonkey

Updated

13 years ago
Blocks: 28586
You need to log in before you can comment on or make changes to this bug.