30375 - Use "busy" cursor while waiting for a link

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Keith Lea]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; N; Win95; en-US) Mozilla/m13
BuildID:    2000022820

when I click a URL in Communicator, I get a nice "watch" cursor (the hourglass)
so I know that I clicked it. in IE and now Mozilla, nothing changes cursor-wise
when I click a link. it's confusing because it's hard to tell whether or not
it's actually doing anything.

Reproducible: Always
Steps to Reproduce:
1. click a link.


Actual Results:  no cursor change!	

Expected Results:  cursor change to 'watch' (the hourglass)

Comment 1

[Ryan Massie]

old build, but still an issue... more of an enhancement thought

Comment 2

[Asa Dotzler [:asa]]

adding 4xp keyword, updating component and owner.

Comment 3

[Brendan Donohoe]

This is a pretty standard behavior and a very important feedback element for

users.  If the machine doesn't respond *immediately* there's no way to know

whether the action was recognized. Consequently, a lot of users will try again

or will try something else.  The throbber isn't good enough, either, because

more often than not, that's not where the user clicked, so that's not what

they're looking at.



It's not that hard to change a cursor...

Comment 4

[Eli Goldberg]

Since it sounds like an actual bug (per Brendan's comments), reassigning to Don, 
and upping severity to 'Normal'.

Comment 5

[Eli Goldberg]

QA Assigning non-confidential New/Assigned User Interface: Design Feedback bugs 
to Matthew Thomas ([e-mail]).

Matthew Thomas is now the QA owner for the User Interface: Design Feedback 
component. (Bugs that involve UI issues in the Netscape-branded Mozilla browser 
should continue be QA assigned to [e-mail].)

Comment 6

[Matthew T (active 1999-2002)]

This is crucial to users' impressions of how fast the browser is generally. I 
have seen many occasions where novice- to intermediate-level Internet Explorer 
users click a link and (due to an occasional IE bug) the cursor isn't changed to 
a busy one. They think nothing has happened, so they click the link again. And 
again, and again. Each time they click, of course, the browser restarts the 
connection process from scratch, resulting in a much longer time taken to 
actually open the page.

Mozilla's throbber and indeterminate progress indicators are more obvious than 
those in IE, but it's still better to get immediate feedback right where you 
clicked, i.e. in the cursor.

Nominating for nsbeta2, since it affects user perception of speed and since it 
should be fairly trivial to implement.

Note that you should use the half-busy cursor here (the combination pointer and 
watch/hourglass), to show that it is still possible to click on another link in 
the page while Mozilla is trying to connect to the first link.

Comment 7

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.

Comment 8

[verah (gone)]

Reassigning as per Don

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

I don't know how to do this. I think there's already a style rule in global that 
may specify this, but if it's not applying, I would have no idea why. 

Back to don. 

Comment 10

[don]

Move to M20 target milestone.

Comment 11

[Matthew T (active 1999-2002)]

This bug could also lead a user into submitting form data twice, which could be
*really* bad. Upping to major severity.

Comment 12

[don]

Matt, can you figure out how to implement this?  If not, please find the
appropriate person to doom.  I agree with Matthew Thomas, there's a big danger
if we're not providing the right kind of feedback here.

Comment 13

[Tim Hill]

Question: How long should the cursor stay busy?  Should it stay busy while the 
entire page is loading, or just until it gets it from the network and the page 
starts loading?  I'm attaching a patch for this that does the former.

I couldn't get CSS to work directly, instead I used the window.setCursor 
method, passing "spinning" as a parameter (spinning is a new cursor in CSS3 and 
represents the hourglass/arrow combination).  It's not perfect -- the browser 
window sometimes tries to set the cursor back to an arrow... but I think it's a 
lot better than nothing.

Comment 14

[Tim Hill]

Attachment: Patch to add busy cursor while loading pages

Comment 15

[Matthew T (active 1999-2002)]

(CCing Tim because I don't see him here.)

When does this patch make the cursor kick in? It should be right after the 
browser has decided to load a page, and right before the DNS lookup starts. Is 
that correct?

Comment 16

[Tim Hill]

Right now it basically turns the hourglass/arrow on and off at the same time 
the throbber is turned on and off.

Comment 17

[Matthew T (active 1999-2002)]

Sorry, Tim, didn't answer your questions.
* IE uses the spinning cursor until the moment the new page starts rendering.
  However, my feeling is that you should continue it until the new page
  finishes loading. (That way it gives a subtle indication that incremental
  reflows might cause the layout of the page to change.)
* Using the mixed pointer-and-hourglass is correct (because you can do other
  things while the page is loading, e.g. click on another link).

W.r.t `the browser window sometimes tries to set the cursor back to an arrow': 
what should be happening is that the `spinning' cursor is used *whenever the 
ordinary arrow cursor would normally be used*, and only in those cases. In all 
places where other cursors are specified (sidebar grippy, page elements 
with :cursor style, etc), those specified cursors should still be applied.

Comment 18

[Matthew T (active 1999-2002)]

Yep, tying it to the throbber makes sense. Duh, I didn't think of that. :-)

Comment 19

[John Dobbins]

Could this be causing some of the double bugs? I'm talking about the cases where 
there are two identical bugs submitted at allmost the same time. Fixing this 
could cut down on the number of dupes. 

Comment 20

[Tim Hill]

The patch turns the cursor to the arrow/hourglass combo over ALL elements in 
the page (and browser), which I think makes sense.  So if a new page is in the 
middle of loading and you mouse over a link, it won't change it to the hand 
pointer.  Once the page is loaded of course, it changes it back to "auto", so 
the cursors then work as expected.  

I saw the "patch" keyword on this briefly, but it disappeared for some reason.  
Matt, are you working on this?  Or perhaps Ben can check this in.

Comment 21

[Matthew T (active 1999-2002)]

Hmmm, ok. This will mean you don't (for example) get the double-arrows cursor 
when mousing over the sidebar grippy while a page is loading ... but loading 
feedback is more important than that, I guess.

[Sorry, I think I removed the patch keyword by mistake in a mid-air collision. 
Reinstating.]

Comment 22

[verah (gone)]

Nav triage team: [nsbeta3+] please double plus this.

Comment 23

[matt]

I have this in my tree and can check in the patch you submitted.
It has been working pretty well for the most part.  Waiting for the green to go 
normal.

Comment 24

[Tim Hill]

I've been running with it for a while too and I've noticed that some sites just 
want to reset the cursor all of the time.  For example, http://www.news.com .  
My patch tries to account for this somewhat, but it's not completely successful.
Also, I kind of miss the hand cursor feedback when moving over a hyperlink 
while a page is loading.  Ideally it would only show the hourglass when 
hovering over the background of the page.  Both of these probably require a 
more complicated solution than a few lines of Javascript code.  Some cursor 
feedback is better than nothing, I suppose, and IE and Nav4 aren't perfect 
either.  I haven't tested this on a Mac or Unix, by the way.

Comment 25

[matt]

patch checked in.
Sorry that took so long.

I do see the problem of the hourglass getting reset all the time.
We should file another bug for that.

Comment 26

[Blake Ross]

I think it's pretty bad that the cursor never changes where it should now after 
you've clicked a link...Is there _any_ better solution to this?

Comment 27

[Blake Ross]

Sorry, I'm reopening this.  We'll have to find a better way to do it; I think 
it's a major loss of feedback that the cursor doesn't change to a hand when 
over a link once you start navigating somewhere.  It seems as if you simply 
can't click the link at all, which isn't true (the same goes for many other 
parts of the UI).

Comment 28

[Tim Hill]

I tend to agree that hyperlink cursor feedback is important.  In the long term, 
any time the cursor is set to an arrow, it should use the arrow/hourglass combo 
instead IF the page is currently loading.  This bug may need to be reassigned; 
I'm not sure who owns the various pieces of code that modify the cursor.  

In the short term I might be able to change this to match IE's behavior, which 
only uses the arrow/hourglass in the content area UNTIL the new page starts 
loading (also, it uses the arrow/hourglass in the chrome area the entire 
time).  I still think it's nice to have the arrow/hourglass feedback while the 
page is loading, but the current solution that hides the hyperlink feedback is 
less than ideal.  Also, having this in JavaScript means embedded Gecko 
applications don't get this basic behavior automatically.

Comment 29

[Matthew T (active 1999-2002)]

See my comments on 2000-07-19: the partially-busy cursor should be `used 
*whenever the ordinary arrow cursor would normally be used*, and only in those 
cases'. Which means, not in the cases where the pointing hand cursor would be 
used.

Is it possible just to piggy-back the case where the ordinary arrow would be 
used, and replace it with the partially-busy cursor?

Comment 30

[Owen Williams]

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

Comment 31

[Decklin Foster]

Right now Linux is using XC_exchange instead of XC_watch, which I find to be
ugly (see bug 48804). This just changed a few days ago. Will a fix for this bug
do something about that? (my guess is yes, so I haven't filed a bug on it.)

Comment 32

[Tim Hill]

In order to replace the arrow cursor with a hourglass/arrow efficiently and in 
all cases, the C++ code needs to be modified.  If Netscape has the resources to 
do this, that would be great.  I might buy a Visual C++ compiler soon to see if 
I could work on it myself, taking a deep breath and saying a few prayers 
beforehand :)

I'm presuming what needs to happen is to add some code around here...
http://lxr.mozilla.org/seamonkey/source/layout/events/src/nsEventStateManager.cp
p#1252
...which adds a special case for using the busy cursor in place of the standard 
arrow whenever the page is loading.  In order to do that, you need to somehow 
communicate with the document in order to retrieve the busy state (not sure 
quite how to do that yet).  Finally you need to make sure the cursor is 
appropriately updated at the beginning and end of the document load.  Sadly I 
can't mentally compile and execute so this is only a guess.

Comment 33

[Tim Hill]

Regarding the Unix cursor.  I agree the cursor used for CSS3 "spinning" in Unix 
is a bit ugly.  The situation will be improved if the hyperlink cursor is used 
for mouseover hyperlinks, but it would still be shown on the page background.  
If you think CSS3 "spinning" should be a different cursor (such as the watch, 
the same as CSS3 "wait"), you should probably file a separate bug.

Comment 34

[matt]

I've been trying to modify the fix for the problem that the cursor
does not change once a new page is loaded.  I don't have time
to change the c++ code the change the cursor in ever possible case
but it would be good at least cover the case you have been talking about.
Doing the other would be a toolkit bug.

Comment 35

[Tim Hill]

Looks like Mac (bug 49173) and Unix people (bug 48839) really don't like the 
busy cursor appearance.  Even if we handle the case for hyperlinks, I'm 
guessing that the cursor may still be annoying for platforms with less pretty 
cursors than Windows.  I suggest we check the window.navigator.platform string 
at startup to either turn off, or special case this behavior for other 
platforms.  The values are "Windows", "OS/2", "Macintosh", and "X11" (Unix).  
I'm not sure what the current behavior is on the Mac and Unix for Navigator4, 
and IE5/Mac.  Perhaps we can use "wait" (the watch) instead of "spinning" for 
those platforms.  And/or consider just using the busy cursor until the new page 
begins loading.

Comment 36

[Matthew T (active 1999-2002)]

IMO we shouldn't special-case it, we should leave it how it is and get bug 48839 
and bug 49173 fixed. Using the `wait' cursor would be misleading the user, 
because it would be implying that user input wasn't possible when it actually 
was.

I don't understand why this bug blocks bug 48839, by the way.

Comment 37

[johng]

nav triage team:
we need to have a final word on what the "right" behavior is.  Matt, Gable and
German should talk about this - reassign to johng for decision.  We need to make
this decision for all related bugs dealing with curser wait indications.

Comment 38

[Tim Hill]

I'm currently testing some C++ code I wrote to support busy cursors in all 
DocShells.  It seems to work fairly well so far.  It basically covers the 
embedding case, chrome windows, browser windows -- anything that uses 
nsDocShell to load content.  For example, the busy cursor is displayed when 
switching between the very slow-loading Preferences panels.

The cursor behavior is as follows...
o Not Busy: 
- Appropriate cursor is used when hovering over elements.
o Busy...
- BEFORE a new page starts loading (i.e. connecting to the network):
"Spinning" cursor is used over ALL elements.  This is necessary so when you 
first click on a link, you know immediately that the new page is loading w/o 
having to move the mouse off the link.  Cursor feedback here isn't really too 
important since the existing page is going away.  Exact same behavior as IE 5.
- AFTER the page starts loading:
"Spinning" cursor only used where the arrow/default cursor would otherwise be 
used.  The hand cursor is shown over hyperlinks, for example.  (I'm wondering 
whether the "spinning" cursor should also take the place of the I-beam cursor 
that is used over selectable web page text?  Otherwise I'm not sure if having 
busy cursor feedback during page load is even that useful, since much of a page 
is often made up of text)

Comment 39

[german]

I agree with Matthew Thomas it should be the partial wait cursor substitutung 
where you would otherwise see the arrow pointer arrow, and only in those areas. 
Meaning that hor/vert grippies should still show the e-w and n-s resize cursors 
even while loading. That is the point of giving loading feedback while stating 
that some areas are still clickable.

Comment 40

[Tim Hill]

Attachment: Patch to remove JavaScript busy cursor hack

Comment 41

[Tim Hill]

Attachment: Patch to nsDocShell to let nsEventStateManager find out the current busy state

Comment 42

[Tim Hill]

Attachment: Patch to nsEventStateManager to use the busy cursor when appropriate

Comment 43

[Matthew T (active 1999-2002)]

Tim, your wondering is correct -- `spinning' should be used during the page load 
even where you are over selectable text. Incremental reflows mean that the text 
may move as the user tries to select it, and using `spinning' here helps warns 
her of this. (But if she starts dragging to make a selection while the page is 
still loading, the I-beam cursor should be used *during the drag*, as it would be 
in the normal case.)

Johng: German has commented here, which presumably means you have had your 
discussion -- so do you want to reassign this back to a programmer?

Comment 44

[Adam Lock]

The patch seems fine to me, but please ensure that it works with situations such 
as the user stopping a load, a load timing out etc.

The cursor should also revert to the normal shape when the pointer is over the 
scrollbar.

Comment 45

[Tim Hill]

I'm fairly certain that the busy cursor will work in those cases, because it 
responds to the same events as the throbber does.  The busy cursor should still 
be used for the scrollbar since it's normally an arrow...
I'm not sure how difficult it would be to use the I-beam while dragging but not 
otherwise.  I'll look into it.

Comment 46

[leger]

Not critical to Netscape 6 ship. Moving to P3.  Adding [PDTP3]

Comment 47

[Blake Ross]

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

Comment 48

[Blake Ross]

Renominating for beta3.  Days before nsbeta3 and not even assigned to an 
engineer yet, this isn't a beta3 stop ship.

it would be appreciated if someone could review those patches, however...cc 
saari for hopeful patch evalation.

Comment 49

[Matthew T (active 1999-2002)]

JohnG, does Germans' 2000-08-24 comment represent the decision of Matt, Gable, 
and German Inc.? If so, please reassign to a programmer. If not, please tell us 
what the delay is. It would be nice if this could get in for M18. Thanks.

Comment 50

[johng]

German's decision is the right one. Reassigning to Don Melton to get help with
triaging. Do we need to fix this for RTM? This does effect the user's perceived
performance of the product.

Comment 51

[don]

We're not gonna take this for beta 3.  Maybe RTM.

Ben, are these patches cool to take at this point?

Comment 52

[don]

Marking nsbeta3-.

Comment 53

[Ben Goodger (use ben at mozilla dot org for email)]

sending to jud. jud, does your group own this code? I tried this patch and it 
works fine for me. 

Comment 54

[Phil Peterson]

PDT marking [rtm-] for N6. Doesn't seem very serious.

Comment 55

[Adam Lock]

Tim, a couple of comments on the patch.

1. Change the style from busyNotBusy to BUSY_FLAGS_NONE etc. and from signed to 
unsigned longs.
2. Consider using flag combinations to make up the various states. e.g. let 
people test for the BUSY_FLAGS_BUSY bit instead of having to compare against 
every busy state, busyPageLoad OR busyBeforePageLoad.

Otherwise it looks fine.

Comment 56

[Blake Ross]

Tim, mind producing an updated version of the patch that addresses Adam's 
comments so we can get it in?

Comment 57

[Blake Ross]

Hmm....

So, I made a patch updated to the tip that addresses most of Adam's comments, 
but I'm not seeing the exact behavior Tim outlined in his 8/24 comment.  
Instead, although the cursor properly changes in the busyBeforePageLoad state, 
it quickly changes back to the standard cursor the second you move the mouse.  
Any ideas?  Some stuff changed in OnStateChange() since this patch was produced 
(STATE_IS_NETWORK -> STATE_IS_DOCUMENT, for example, although that had no 
effect).  I'll post the updated patch if you want, but it's kind of useless 
with the aforementioned problem.

Comment 58

[Judson Valeski]

-> adam

Comment 59

[Blake Ross]

really -> adam

Comment 60

[Adam Lock]

Blake, can you provide me with your later patch?

Thanks

Comment 61

[Adam Lock]

Attachment: Patch against latest source with recommended changes

Comment 62

[Adam Lock]

I've updated the patch slightly to work with the latest source code and to pick 
up some of the changes I suggested. I'll test it a little more tomorrow and if 
no one has any objections it will be submitted for review.

Comment 63

[timeless]

sorry to be a grouch...
+   }
+   else if ((aStateFlags & STATE_STOP) && (aStateFlags & STATE_IS_NETWORK))
+   {
...
+     if(mainWidget)
+       {
+       mainWidget->SetCursor(eCursor_standard);
+       }

let's pick an indentation style and maintain it.
the one i prefer is:
if (cond) {
  statement;
} else {
  statement;
}
<!-- //the only case that does not follow this style is functions:
void functionName ( ... )
{
  body;
}
-->
i think that:
+   else if ((aStateFlags & STATE_STOP) && (aStateFlags & STATE_IS_NETWORK))
can be rewritten as
+   else if (aStateFlags & STATE_STOP && aStateFlags & STATE_IS_NETWORK)
however i'll leave that for brendan

Comment 64

[Brendan Eich [:brendan]]

timeless wrote:
>i think that:
>+   else if ((aStateFlags & STATE_STOP) && (aStateFlags & STATE_IS_NETWORK))
>can be rewritten as
>+   else if (aStateFlags & STATE_STOP && aStateFlags & STATE_IS_NETWORK)
>however i'll leave that for brendan

It's not a big deal, but I prefer unnecessary parenthesization of & against &&
for readability's sake (without the parens, the &&&s are hard to tell apart).

However, there's no need for a separate mask and test, and the branch implied by
&&, to test whether two bits are set:

+   else if ((aStateFlags & (STATE_STOP|STATE_IS_NETWORK)) ==
(STATE_STOP|STATE_IS_NETWORK))

Notice that parens are required now, due to C and C++'s notorious botched
precedence of equality operators as higher than the bitwise logicals.

Even shorter and faster:

+   else if ((~aStateFlags & (STATE_STOP|STATE_IS_NETWORK)) == 0)

/be

Comment 65

[Adam Lock]

Attachment: Updated patch

Comment 66

[Adam Lock]

I've updated the patch to include suggestions. I consider the original bitfield 
testing to be more readable but I've changed it since a more efficient way was 
suggested. Also note that docshell is a mess of coding conventions so no style 
would be correct for this file (many blocks are indented by 3 spaces!) but I've 
changed the brace style anyway.

This patch is now ready for review.

Comment 67

[timeless]

filed bug 66434 for cleanup of nsDocShell.cpp

Comment 68

[Brendan Eich [:brendan]]

Adam: did you mean to leave this brace-style instance (and one like it soon
after) alone?

!     if(mainWidget)
! 	{
!       mainWidget->SetCursor(eCursor_spinning);
! 	}

Also, please use diff -u.  Sorry to nag, I don't have more to add right now.

As http://www.mozilla.org/hacking/reviewers.html, Rule 20, makes clear, bad or
no ownership made manifest by crappy/ugly/inconsistent style is a bug to be
fixed, incrementally or all at once, by new and better owners.  You should go
strong, get rid of Travis's evil Fibonacci-number indentation, and be a
righteous owner.

/be

Comment 69

[Adam Lock]

Fix is checked in now. Coding style issues in docshell are covered by that other 
bug.

Comment 70

[Simon Fraser [no longer active]]

Who let this go by:

  if ((~aStateFlags & (STATE_START | STATE_IS_NETWORK)) == 0)

which is equivalent to the much more easily comprehended

  if (aStateFlags & (STATE_START | STATE_IS_NETWORK))

Comment 71

[Adam Lock]

No it isn't, 

if ((~aStateFlags & (STATE_START | STATE_IS_NETWORK)) == 0)

Is equivalent to:

if ((aStateFlags & STATE_START) && (aStateFlags & STATE_IS_NETWORK))

I changed the expression from the latter to the former following comments from 
Brendan. See above.

Comment 72

[Simon Fraser [no longer active]]

Oops, you're right.

However, I find the
if ((aStateFlags & (STATE_STOP|STATE_IS_NETWORK)) == (STATE_STOP|
STATE_IS_NETWORK))
or 
if ((aStateFlags & STATE_START) && (aStateFlags & STATE_IS_NETWORK))
forms much easier to comprehend. Smaller brain-print.

(BTW, this error I made does not affect my patch in bug 67084, where I note that 
all this needs to be refactored anyway).