Closed
Bug 50656
Opened 24 years ago
Closed 24 years ago
window.defaultStatus not working
Categories
(SeaMonkey :: UI Design, defect, P2)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: rcherny, Assigned: law)
References
()
Details
(Keywords: regression, testcase, top100, Whiteboard: [nsbeta3-][rtm++]Fix on trunk, reviewed and approved)
Attachments
(2 files)
1.18 KB,
text/html
|
Details | |
1.06 KB,
patch
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/4.7 [en] (WinNT; U)
BuildID: 2000082808
window.defaultStatus is not working. set via onLoad the window status bar
should display the requested string text. with an onMouseOver event setting
window.status text, it should override the window.defaultStatus display. after
the onMouseOver event, when a user mouses out, it should default back to the
window.defaultStatus text (onMouseOut is implied).
Reproducible: Always
Steps to Reproduce:
create a page with onload="window.defaultStatus='some text';return true" in the
<body> tag. Also create link on page with an onMouseOver window.status set.
Actual Results: no text displayed in status bar onLoad. When mouse passed over
link, the onMouseOver text does display. OnMouseOut, the window.status remains
as default status message.
Expected Results: first, onload, it should display the window.defaultStatus
text string. onMouseOver does display it's message text, but the message
remains onMouseOut (onMouseOut should be implied and the defaultStatus should
take over - this was the way it worked in NS 3.x-4.x, and IE4-5.x)
Comment 1•24 years ago
|
||
Confirmed with 2000-09-12-12 on Linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•24 years ago
|
||
AFAIK the DOM code does the right thing, reassigning to the docshell people
since the DOM code relies on the docshell for setting the default status. Also
nominating for beta3 since this is a regression that needs to be fixed.
Assignee: jst → adamlock
OS: Windows NT → All
Priority: P3 → P2
Hardware: PC → All
This is not the docshell at fault here. Calls to set the status, default status,
and link are all getting through to the implementation in navigator.js. The
implementation of setJSDefaultStatus is here:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator
.js#230
I'm attaching a modified version of the sample that demonstrates status and
default status both get through.
Reassigning to XP Apps
Assignee: adamlock → don
Component: DOM Level 0 → XP Apps
QA Contact: desale → sairuh
Updated•24 years ago
|
QA Contact: sairuh → claudius
Comment 5•24 years ago
|
||
P3, nsbeta3-, assigning to law. Doesn't seem that bad. don suggests cc'ing
brendan in case he has feedback.
Assignee: don → law
Priority: P2 → P3
Whiteboard: [nsbeta3-]
Bill, how BAD is this bug anyway? I'm disturbed that it's a regression, but can
we actually ship without this? Should it be nominated for rtm?
Reporter | ||
Comment 7•24 years ago
|
||
so something I've been setting in browsers since JavaScript began in Netscape
2.0, "onload=window.defaultStatus" now won't work in Netscape 6? are we going
forward or going backwards? I know it isn't a big deal but if our pages are
going to be resembling apps more and more it seems rather significant. I know
it is trivial in some respects when weighed against others, but small things
like this are frequently what products are judged on ...
Not that I have my hands in the code under the hood (not a C programmer), but
based on some of the attachments here it seems that the code is generally
working for defaultStatus, something's just being missed off the onload handler
or something. Is that really a difficult fix?
It seems like it wouldn't be too hard to fix. Adam says we're getting the right
data to navigator.js. So it appears to just be a problem getting it into the
status bar.
Might be a symptom of that bug about the load listeners: them firing could
trigger us to set (default) status to "Document: Done". But that's just a
theory. Might fit with the onload aspect of this report (onload handler is now
called just before we show "Document: Done" where previously it was called just
afterwards. Again, just a theory.
Rob, you're right. This is ugly. But if you had all my bugs, you might not
think it was quite so ugly. How about you put some dump() calls in navigator.js
and help us figure out what's happening? I can't see giving it to much
attention myself (but I'd be glad to help you, or somebody else, work on it).
Reporter | ||
Comment 9•24 years ago
|
||
sorry about my tone it was early/late - wasn't trying to flame. ;) yeah, i
know with the volume of bugs you have to pick your battles.
i'll check out the nav.js file but i can't make any promises - that will
officially be the first mozilla code i've hacked ... and i haven't done any
work with dump() yet either ... but i'll look.
Comment 10•24 years ago
|
||
Nominating this for rtm so that it gets re-evaluated in case the fix can't make
beta 3...
Keywords: rtm
Assignee | ||
Comment 11•24 years ago
|
||
Rob, dump() is just alert() that goes to the console. navigator.js is a good
place to start to understand Mozilla; sort of the crossroads where all the
technologies come together (i.e., "collide"). Don't hesitate to bug us for help
if you need it.
Comment 12•24 years ago
|
||
Upping priority to P1, not fixing this for rtm would simply be silly,
defaultStatus is used *alot*, adding top100 keyword.
Keywords: top100
Priority: P3 → P1
Reporter | ||
Comment 13•24 years ago
|
||
could this have anything to do with work done on bug 31997 ?
Assignee | ||
Comment 14•24 years ago
|
||
Perhaps related to bug 31997, but more likely just coincidence that the both
deal with window.status in one way or another. That one seemed specific to
resetting the status upon page load.
Comment 15•24 years ago
|
||
nav triage team:
RTM NEED INFO so when we get a patch we can + it. P2
Priority: P1 → P2
Whiteboard: [nsbeta3-] → [nsbeta3-][RTM NEED INFO]
Assignee | ||
Comment 16•24 years ago
|
||
Here's the scoop (as I see it, at least):
First, the navigator.js code that decides what to display in the status bar (see
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#192)
is quite simple. There are 4 sources for this text and this UpdateStatusField
code simply uses the first that it finds (i.e., is non-null), in this priority:
1. jsStatus - This is the navigator.js version of the window.status DOM level 0
property, as set by the web page; this has top priority so that setting
window.status will change the status and override jsDefaultStatus/defaultStatus
(see below)
2. overLink - This is the URL of any moused-over links; it is set by some
webshell/docshell notifier mechanism (which essentially calls into navigator.js
when the user mouses in/out of links). This is set to null when the user mouses
out of a link (which is pertinent for one of the bugs, see below).
3. jsDefaultStatus - navigator.js store for the window.defaultStatus DOM level 0
property; this is updated when scripts set window.defaultStatus
4. defaultStatus - this is a variable that holds what I'd characterize as
browser "auxilliary" status messages (such as "Document: Done" and others of
that ilk).
So the strategy is that the browser shows it's auxilliary msg unless and until
the page sets some other msg (via window.status or window.defaultStatus), or,
the user mouses over a link.
There are specific functions in navigator.js that get called to update each of
the first 3 variables. Each calls UpdateStatusField after storing the new value
(which makes sense as what is being shown can change whenever any of these 3
variables change state). UpdateStatusField is also called when networking
notifications are received (which corresponds with the defaultStatus variable
changing value).
After adding some debugging code and observing what happens when I go to this
bug's attachment, I see two distinct bugs:
1. The setting of window.defaultStatus (i.e., navigator.js's jsDefaultStatus
variable) does not "take" because code in the progress notificatioan handling
code uses setOverLink(msg) that has the effect of mousing over a link to
"Document: Done." The line of code in question is at:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#329
I think this line of code is in error. If I comment it out, things work just
fine: the window.defaultStatus setting in that page's onload handler does what
it is supposed to. And I don't see any other problems.
That line of code seems to have replaced the commented-out lines that precede it
(window.nsXULWindow.setDefaultStatus(msg);). I don't know what that line was
supposed to do. It seems guaranteed to break any script that set
window.defaultStatus; probably it fixed something funky so that "Document: Done"
actually showed up or something like that.
2. The second bug is that when the user mouses *out* of the link with the
onMouseOver handler, the status field does not revert to
jsDefaultStatus/defaultStatus (as it should, based on empirical evidence of
Communicator and IE, and based on text in the JS book I'm using for reference,
The Definitive Guide, 3rd Edition). The reason is that when JS code sets
window.status, we store that in navigator.js's jsStatus variable and that gets
top priority until such time as JS code resets window.status. The problem is
that window.status is supposed to be much more transient than that. As The
Definitive Guide says: "it is likel to be immediately overwritten by a browser
message such as "Document: done". In other words, setting window.status should
only cause the status bar to change until such time as some other
status-msg-affecting event transpires; e.g., mousing out of the link.
To fix *that*, I simply inserted the line:
jsStatus = null;
inside setJSStatus, just after the call to UpdateStatusField().
With that change, that function now:
1. Updates jsStatus (to null or to what was assigned).
2. Calls UpdateStatusField. This forces the value assigned previously at step 1
to appear on the status bar (presuming it wasn't null).
3. Then sets jsStatus to null. This means jsStatus won't override the other
settings the next time UpdateStatusField() is called.
When all is said and done, a 2 line of code patch to navigator.js seems to fix
things. I'm a little leery of the risk of this patch but I'll throw it out
there for people to try out. I'll even try to get it reviewed and approved by
the module owner (Ben?).
I'll attach the patch shortly (I need to separate that change from other patches
I have to navigator.js at the moment).
Assignee | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
I tried this, and created a test case that used defaultStatus, and the patch
works fine.
r=ben.
Assignee | ||
Comment 19•24 years ago
|
||
I analyzed the risk factor in that patch and I think it's OK. Here's the
rationale that I previously mailed out to some folks here when I asked them to
review it:
1. Removing the completely bogus calls to setOverLink is harmless. Since we've
put the msg into defaultStatus, we know that text will be used unless some higher
priority value is set (jsStatus, overLink, or jsDefaultStatus). Putting it in
overLink can only have an effect if jsStatus or jsDefaultStatus is set. jsStatus
is never set (with the second patched line, see below). If jsDefaultStatus is
set, then we actually want to use it (that's bug #1).
2. Resetting jsStatus to null after the call to UpdateStatusField is OK since
that call will have ensured that the text appears in the status bar, at least
for a fleeting moment. It will only disappear if something else causes
updateStatusField to be called. But in that case, the status field *should* be
updated, as per observed behavior of Communicator, IE, and the "spec"
(JavaScript, The Definitive Guide) and that is precisely bug #2.
Comment 20•24 years ago
|
||
PDT, the fix is now reviewed and approved.
Whiteboard: [nsbeta3-][RTM NEED INFO] → [nsbeta3-][rtm+]Fix in hand, reviewed and approved
Comment 21•24 years ago
|
||
r=matt
this looks good to me bill
Comment 22•24 years ago
|
||
PDT marking [rtm need info] after talking to law on the phone. He's going to
land on the trunk and see how it goes before landing on the branch.
Whiteboard: [nsbeta3-][rtm+]Fix in hand, reviewed and approved → [nsbeta3-][rtm need info]Fix in hand, reviewed and approved
Assignee | ||
Comment 23•24 years ago
|
||
Fix checked into the trunk.
I looked into my remaining concern, which was that the setOverLink() call to
elevate the priority of auxilliary messages such as "Document: Done" might be
masking a bug whereby those messages wouldn't appear otherwise. The fear was
unfounded, as it turns out. That call does not mask any such bug. Not that
there isn't such a bug (see bug 47359). But that line of code certainly doesn't
mask it.
Status: NEW → ASSIGNED
Comment 24•24 years ago
|
||
PDT folks, this fix is on trunk. As near as we can tell, civilization did not
come to an end. :-) Nor did anything else bad happen, at least from this fix.
See Bill's previous comment on his former concerns, which are not an issue anymore.
Whiteboard: [nsbeta3-][rtm need info]Fix in hand, reviewed and approved → [nsbeta3-][rtm+]Fix on trunk, reviewed and approved
Comment 25•24 years ago
|
||
rtm++.
Whiteboard: [nsbeta3-][rtm+]Fix on trunk, reviewed and approved → [nsbeta3-][rtm++]Fix on trunk, reviewed and approved
Assignee | ||
Comment 26•24 years ago
|
||
Fix checked into branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 27•24 years ago
|
||
VERIFIED FIxed branch and trunk builds 2000102008
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 28•23 years ago
|
||
I'm confused. Not to beat an incredibly old and dead horse, but ... I suppose
where I'm confused is -- shouldn't the correct behavior reset back to
defaultStatus when you mouseOff a link? That's what NS2-4 / IE do ... Here with
the current patch (on the original test case) it stays on the mouseOver setting.
Am I wrong?
moz 1.1a
http://www.cherny.com/mozilla/status.html
Comment 29•23 years ago
|
||
Claudius, should this be reopened, or a new bug filed?
/be
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•