Remove 'Done' in browser statusbar after first link hover

RESOLVED FIXED

Status

RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: jrgmorrison, Assigned: jrgmorrison)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

16 years ago
This is a follow-on to bug 48436. I'd noticed that with the change to just say
'Done' after the document is loaded, there is this odd feedback when you
mouseover then mouseout a link. The status toggles to the link URL and then
reverts to saying 'Done' again.

User: "Done? Done what? Did something just happen?"

I'm fine with just saying 'Done' [instead of 'Document: Done (nnn)'], but we
should just clear that string when the user hovers the first link. [By the
way, this is what IE does].

Patch to follow (which also removes an unused argument to setOverLink().
(Assignee)

Comment 1

16 years ago
Created attachment 107256 [details] [diff] [review]
patch to clear this.defaultStatus (and remove unused argument from setOverLink()).
(Assignee)

Comment 2

16 years ago
Comment on attachment 107256 [details] [diff] [review]
patch to clear this.defaultStatus (and remove unused argument from setOverLink()).

r=/sr= requested (0.00372 secs.)
Attachment #107256 - Flags: superreview?(blaker)
Attachment #107256 - Flags: review?(caillon)
(Assignee)

Comment 3

16 years ago
Created attachment 107260 [details] [diff] [review]
a microscopically better patch

Maybe we should just drop the empty string from navigator.properties. 

(But what about the navigator.properties files in   
  /l10n/langpacks/en-DE/chrome/en-DE/navigator/locale/ and
  /l10n/langpacks/en-DE/chrome/en-GB/navigator/locale/?
Is that just dead ****?)
(Assignee)

Comment 4

16 years ago
(If we go with patch 2, I guess I need to fix navigator.properties in the
commercial tree too).
Comment on attachment 107260 [details] [diff] [review]
a microscopically better patch

- Why not initialize the doneString in init() ?

- Now you are getting the done string twice; there is another caller of it,
which is where the defaultStatus gets set to the done string.  If you are
caching it into a variable (which I think is probably a good idea so we don't
need to fetch it for every page load), please remove other caller (at around
line 267) and use the cached value.

- Don't you also want to clear other messages from the default status? 
(Stopped, Timeout, etc?)
(Assignee)

Comment 6

16 years ago
Comment on attachment 107256 [details] [diff] [review]
patch to clear this.defaultStatus (and remove unused argument from setOverLink()).

Ah, yeah. I'm gonna to prepare a different patch
Attachment #107256 - Attachment is obsolete: true
(Assignee)

Comment 7

16 years ago
Comment on attachment 107256 [details] [diff] [review]
patch to clear this.defaultStatus (and remove unused argument from setOverLink()).

Hmm, the patch manager doesn't automagically remove review requests from
obsolete bugs (so do it manually)
Attachment #107256 - Flags: superreview?(blaker)
Attachment #107256 - Flags: superreview?
Attachment #107256 - Flags: review?(caillon)
Attachment #107256 - Flags: review?
(Assignee)

Comment 8

16 years ago
Comment on attachment 107256 [details] [diff] [review]
patch to clear this.defaultStatus (and remove unused argument from setOverLink()).

Sigh.
Attachment #107256 - Flags: superreview?
Attachment #107256 - Flags: review?
(Assignee)

Comment 9

16 years ago
Created attachment 107334 [details] [diff] [review]
revised patch; use a flag to determine when first overlink occurs and then clear the defaultstatus

Two questions:

1) is this just a dead member variable?
    http://lxr.mozilla.org/seamonkey/search?string=statusTimeoutInEffect

2) I can make this cache the 'Done', 'Timeout' and 'Stopped' strings, 
   but isn't that just swapping one hash table lookup for another one
   (i.e., is it really faster to retrieve a JS variable than it is 
   to pull it out of the string bundle)?
Attachment #107260 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Comment on attachment 107334 [details] [diff] [review]
revised patch; use a flag to determine when first overlink occurs and then clear the defaultstatus

(Oops. I'll fix that TAB).
Comment on attachment 107334 [details] [diff] [review]
revised patch; use a flag to determine when first overlink occurs and then clear the defaultstatus

>Index: resources/content/nsBrowserStatusHandler.js

>+  didFirstOverLink : false,
>+

No need for this variable (See below).


>@@ -147,9 +146,14 @@
>     this.updateStatusField();
>   },
> 
>-  setOverLink : function(link, b)
>+  setOverLink : function(link)
>   {
>     this.overLink = link;
>+    // clear out 'Done' (or other message) on first hover
>+    if (!this.didFirstOverLink) {
>+      this.defaultStatus = "";
>+      this.didFirstOverLink = true;
>+    }

You can just unequivocally set the defaultStatus to "" here.
Attachment #107334 - Flags: superreview?(jaggernaut)
Attachment #107334 - Flags: review+
(Assignee)

Comment 12

16 years ago
Created attachment 107411 [details] [diff] [review]
le patch encore fois; just clear this.defaultStatus in setOverlink if it has a value

Try this one.
Attachment #107334 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107334 - Flags: superreview?(jaggernaut)
(Assignee)

Updated

16 years ago
Attachment #107411 - Flags: superreview?(jaggernaut)
Attachment #107411 - Flags: review?(caillon)
Comment on attachment 107411 [details] [diff] [review]
le patch encore fois; just clear this.defaultStatus in setOverlink if it has a value

>+  setOverLink : function(link)
>   {
>     this.overLink = link;
>+    // clear out 'Done' (or other message) on first hover
>+    if (this.defaultStatus)
>+      this.defaultStatus = "";
>     this.updateStatusField();
>     if (link)
>       this.statusTextField.setAttribute('crop', 'center');

I still contend that you should not check to see whether or not we have a
value, and just always set it to "".  No matter what.  You don't win anything
with the check.
Attachment #107411 - Flags: review?(caillon) → review+
(Assignee)

Comment 14

16 years ago
I'm just trying to avoid re-setting that property to "" again and again as 
you move the mouse over a page. But maybe that doesn't add up to anything 
anyways.
(Assignee)

Comment 15

16 years ago
These look like they are dead lines too. Can I remove these? (They were used
to "ignore local/resource/chrome files", but that has moved into 
nsBrowserStatusFilter.cpp.


@@ -37,11 +37,6 @@
  *
  * ***** END LICENSE BLOCK ***** */

-const NS_ERROR_MODULE_NETWORK = 2152398848;
-const NS_NET_STATUS_READ_FROM = NS_ERROR_MODULE_NETWORK + 8;
-const NS_NET_STATUS_WROTE_TO  = NS_ERROR_MODULE_NETWORK + 9;
-
-
 function nsBrowserStatusHandler()
 {
   this.init();
@@ -78,8 +73,6 @@
If it's already "" it shouldn't add up to anything more than getting the value
of it, and (implicitly) converting it to a boolean value.
Yeah, looks like you can remove the other stuff too.  r=caillon to do that.
I had jrgm profile the difference with and without the check, and apparantly
using the check is indeed ever so slightly faster, so we'll be using that.

Comment 19

16 years ago
Comment on attachment 107411 [details] [diff] [review]
le patch encore fois; just clear this.defaultStatus in setOverlink if it has a value

sr=jag, and yes to comment 15 (so please add that)
Attachment #107411 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Comment 20

16 years ago
Checking in xpfe/browser/resources/content/nsBrowserStatusHandler.js;
/cvsroot/mozilla/xpfe/browser/resources/content/nsBrowserStatusHandler.js,v  <--
 nsBrowserStatusHandler.js
new revision: 1.46; previous revision: 1.45
done
Checking in xpfe/browser/resources/locale/en-US/navigator.properties;
/cvsroot/mozilla/xpfe/browser/resources/locale/en-US/navigator.properties,v  <--
 navigator.properties
new revision: 1.73; previous revision: 1.72
done

I still need to do the checkin in the commercial tree for navigator.properties.

Comment 21

16 years ago
the removal of not-quite-dead-yet code (comment 15) has resulted in bug 182184

Comment 22

16 years ago
was this the reason for bug 182249 as well?

Comment 23

16 years ago
*** Bug 163330 has been marked as a duplicate of this bug. ***

Comment 24

16 years ago
The known/possible regressions have been fixed, so has the original bug (it
works and the CVS log shows no modification after checkin).

Does anything prevent this bug from being resolved?
marking fixed because it is.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Product: Core → Mozilla Application Suite

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.