40838 - disallowing "change status bar text" often leaves status bar empty for links (onmouseover="return true;" --> status bar doesn't show url for links)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Christos Cheretakis]

after entering service and seeing the tell-a-friend button in bottom "mood-bar"
all mood buttons, when onmouseovering show their url in status bar
taf, the last one, doesn't show anything --just "document done"
tested on linux-2000052520

Comment 1

[Christos Cheretakis]

Attachment: saved test-case, should use images, I suppose

Comment 2

[Jesse Ruderman]

fixing url so it isn't a porn site :)

Comment 3

[Jesse Ruderman]

Attachment: same testcase, with <base href...> so that images show up

Comment 4

[Jesse Ruderman]

Attachment: simpler testcase

Comment 5

[Jesse Ruderman]

confirming (2000 052615 on win98), updating summary, etc.

related: bug 38380 (another onmouseover="return true;" bug)

Comment 6

[joki (gone)]

Okay, I'm not entirely sure I understand the issue here but the all the test 
cases seem to be performing as I would expect.  The test case which has 'return 
true' for all of the mouseover handlers will correctly not show a url.  In the 
other test cases which don't have 'return true' in the mouseovers we correctly 
show the url.  I'm going to mark this WORKSFORME but if I'm missing something 
here please reopen this.

Comment 7

[Christos Cheretakis]

Still doesn't work for me! 4.73 on the same page shows urls in status bar,
linux/2000061220 mozilla does nothing. Checked the source, there's no return
true, there aren't any evetns specified for the moods at all!

Comment 8

[Jesse Ruderman]

>The test case which has 'return true' for all of the mouseover handlers will 
>correctly not show a url.  In the other test cases which don't have 'return 
>true' in the mouseovers we correctly show the url.  

You seem to agree with what mozilla is doing.  Why is what mozilla is doing 
correct, though?

Comment 9

[joki (gone)]

That is how onmouseover handlers work.  If you have an onmouseover handler which 
ends with a 'return true' the browser will not show the URL for the link.  If 
you have an onmouseover handler which ends without a return value or with 
'return false' the browser will show the url.  This has been true since I 
believe Netscape 2.0, definately since 3.0.

I'm marking this fixed again now, else I will likely forget to close it again.  
If you think the browser is not behaving as I've described above please reopen 
it.

Comment 10

[Christos Cheretakis]

Is it just me? The page does NOT have onmouseover events specified, which means,
mozilla HAS TO SHOW the url when mouse is over the images in the bottom part.
However, mozilla DOES NOT show them. Mind you, this IS a bug.
Currently using 061311 in linux

Comment 11

[joki (gone)]

Well there is a mouseover but its on the image.  Which I guess is the 
real question, now that images can get mouseovers and those events bubble up to 
the link do we keep using the same rule that 'return true' prevents the display 
of the url?

Comment 12

[Christos Cheretakis]

Oops! You're right! I just checked the some of the first images that don't have
onmouseovers. Earlier builds of mozilla used to do the same thing with all the
buttons. Sorry for shouting!...

Comment 13

[joki (gone)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 14

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 15

[Loco]

Updating QA Contact.

Comment 16

[Mike Young]

Attachment: Testcase with text along with onmouseover in image

Comment 17

[Mike Young]

I modified the attachment demonstrating the problem this bug deals with. This
bug is very confusing so far... I think the main question comes down to whether
onmouseover in an image should remove the url from the link. We admit that when
onmouseover is in a link there should be no URL. This is the case in the
examples. I'm fine with the onmouse over in an image tag affecting the "URL in
status bar" of its parent. As long as the text in the first example retains the
URL, then the parent-child relationship works. If a user hovers over the image
with a "return true" statement then all status messages from its parents should
also be affected.

Hopefully this bug can be resolved since its workings are what is expected.
(BTW: is there a W3C standard for this? I couldn't find one specifically) 

Comment 18

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 19

[Madhur Bhatia]

QA contact updated

Comment 20

[Mike Young]

This bug no longer exists. The testcases now show the URL for all links whether
images or not. onmouseover="return true;" has no effect on the status bar.
(Which is as I now see it, is the proper way to go. unless we specifically
mention window.status there should be no change to the status bar.)

Marking WORKSFORME

if anyone objects, please reopen.

Comment 21

[Jonas Jørgensen]

Reopening (was just about to file this one myself).

Using build 2002050306 from the 1.0 branch, none of the links in attachment 9272 [details]
show the URL in the status bar when I hover over them.

Comment 22

[Nicolas PENINGUY]

I confirm (today's build of the 1.0.0 branch under Linux)

Comment 23

[Cesar Eduardo Barros]

Found it on http://manga.flyswatter.ca/main.html too. It is annoying - it looks
like mozilla is stuck.

Also, I have "Change status bar text" disabled on the prefs, so the site
shouldn't be able to do anything which would affect the status bar. Including
"return true".

I think the page is using the "return true" to prevent the browser from
overwriting the text it just set using window.status:

onmouseover="window.status='** page policies **' ;return true"

Else it would be overwritten by the URL.

Comment 24

[paxunix]

I also browse with the "Change status bar text" pref
(dom.disable_window_status_change) enabled (i.e. don't let scripts change the
status bar text) and to me the expected behavior should then be that the status
bar will only ever show the URL when hovering over a link.  It seems that what
happens is that setting window.status is a no-op, but the event handler is
allowed to return true, ensuring that no URL appears.

I would think that such behavior is still allowing a script to change the status
bar, even if the change is effectively setting the status bar text to nothing.  

Is a suitable fix to make every call that sets window.status while the above
pref is enabled instead set the status bar text to the underlying URL?  Then the
return value from the event handler is irrelevant and any other processing done
in the event handler can remain unaffected.

Comment 25

[Jonas Jørgensen]

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

Comment 26

[Henry James]

My remark from bug #166923:

Strangely. The link URL *is* shown correctly if
"capability.policy.default.Window.status" is set to "noAccess" in prefs.js. I
cannot image this is by intention. If it is, I consider it wrong.

Comment 27

[Kenneth Herron]

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

Comment 28

[Kenneth Herron]

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

Comment 29

[Jeremy M. Dolan]

> Strangely. The link URL *is* shown correctly if
> "capability.policy.default.Window.status" is set to "noAccess" in prefs.js.

So can't we do away with dom.disable_window_status_change, and have the UI set
this the CAPS one? It seems to do what everyone wants. Having a blank sbar is
worse than letting the link set it. At least that way you can see the section
name usually.

Comment 30

[Jonas Jørgensen]

> So can't we do away with dom.disable_window_status_change, and have the UI set
> this the CAPS one?

Not until bug 122866 is fixed. CAPS currently throws an exception, causing the
script to stop executing. See discussion in bug 117707.

Comment 31

[Andrew Schultz]

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

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

I guess taht the caps way _only_ works _because_ it throws, because the JS will
not be executed further and the return will not be reached.

To solve this - how about moving the generic HandleDOMEvent call at
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#1416
to the end of the function, so the statusbar will always contain a status text?

Comment 33

[Jesse Ruderman]

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

Comment 34

[Boris 'pi' Piwinger]

There is some quite important site which shows the problem, search results on
AlltheWeb.com, e.g.: http://www.alltheweb.com/search?q=bugzilla

pi

Comment 35

[Jesse Ruderman]

To clarify what pi said, this bug affects Alltheweb *if you have disallowed
scripts changing window.status*.

Comment 36

[Pham]

.

Comment 37

[Vlad Berditchevskiy]

There is another page that demonstrates this issue:

http://java.sun.com/j2se/1.4.1/download.html

On that page there seem to be no JavaScript mouse hooks at all, but when whe
mouse poiter moves over the "DOWNLOAD" image, the status bar shows nothing.
Actually, it is not really a link, it's some kind of "form submit button", so is
that a bug or not? Anyway, it would be nice to see something useful on the
status bar, so even if it is supposed behavior, please consider it as a feature
request.

Comment 38

[black-hole]

Same problem on several pages. (I checked some of the examples with my Moz 1.5
rc W98 and they "worked" as described by the contributors. (text and image links
not working, flash neither, but that's flash of course.)
I also have disabled JavaScript to change status bar text etc..
Copying link location & pasting in editor/url-field works so it is a matter of
beauty but it would still be fine to have that one fixed someday. 
By the way: Those java sun downloadbuttons are small gifs and there is no copy
link location option for them. 

Comment 39

[Norbert Wienholz]

Attachment: Different link behaviours, even after prefs.js setting

Comment 40

[Norbert Wienholz]

Comment on attachment 136498 [details]
Different link behaviours, even after prefs.js setting

Changing the 'capability.policy.default.Window.status' fixes the behaviour if
there are more than one JS statements in the mouseover action. If it only
contains a 'return true' the link is still hidden.

Comment 41

[Jesse Ruderman]

Norbert: that's because capability.policy.default.Window.status makes
"window.status='foo';" throw an exception (bug 122866), as explained in comment 32.

Comment 42

[Jon Henry]

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

Comment 43

[Jon Henry]

Perhaps this is obvious, but from the bug I just duped (bug 227159), the same
behavior can be seen in Firebird if you disable JavaScript entirely.

Also, is Saari still the correct assignee for this bug, or should it be
reassigned to the default address?

Comment 44

[Dave Evans]

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

Comment 45

[Johnny Stenback (:jst)]

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

Comment 46

[bomfog]

(In reply to comment #38)

> Copying link location & pasting in editor/url-field works so it is a matter of
> beauty but it would still be fine to have that one fixed someday. 

Another work-around: right-clicking the link shows the URL in the status bar,
until the mouse moves off the link.

Comment 47

[Phil Ringnalda (:philor)]

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

Comment 48

[preph2005]

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

Comment 49

[Michael Newton]

"If you have an onmouseover handler which ends with a 'return true' the browser
will not show the URL for the link... This has been true since I believe
Netscape 2.0, definately since 3.0."

From a programming point of view, can anyone say why this is rational behaviour?
 The status bar isn't even mentioned in the javascript, just 'return true' which
makes absolutely no sense to me.

FWIW I'll second comment 24.

Comment 50

[Stefan Borggraefe]

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

Comment 51

[OstGote!]

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

Comment 52

[Josh Birnbaum]

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

Comment 53

[Grant Wohl]

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

Comment 54

[Florian Quèze [:florian]]

Attachment: patch v1

Comment 55

[Johnny Stenback (:jst)]

Comment on attachment 195783 [details] [diff] [review]
patch v1

   if ((*aEventStatus == nsEventStatus_eIgnore ||
-	(*aEventStatus != nsEventStatus_eConsumeNoDefault &&
+	((*aEventStatus != nsEventStatus_eConsumeNoDefault ||
+	  nsContentUtils::GetBoolPref("dom.disable_window_status_change")) &&
	 (aEvent->message == NS_MOUSE_ENTER_SYNTH ||
	  aEvent->message == NS_MOUSE_EXIT_SYNTH))) &&
       !(aFlags & NS_EVENT_FLAG_CAPTURE) && !(aFlags &
NS_EVENT_FLAG_SYSTEM_EVENT)) {

The order inside the part of the condition you're changing could be changed so
that we don't check the pref unless we're dealing with a enter or exit event
since the pref check isn't all that cheap.

r=jst, bz should sr once you have an updated patch.

Comment 56

[Florian Quèze [:florian]]

Attachment: patch v2

Comment 57

[Boris Zbarsky [:bzbarsky]]

So:

1)  Why does this work?
2)  Why is this change only needed for HTML?

Comment 58

[Florian Quèze [:florian]]

(In reply to comment #57)
> So:
> 
> 1)  Why does this work?
The function HandleDOMEventForAnchors tries script event handlers first. If the
script returned true, *aEventStatus is set to nsEventStatus_eConsumeNoDefault
before we get to the test my patch changes.
If the value of *aEventStatus was nsEventStatus_eConsumeNoDefault the link url
wasn't put to the status bar.
If the pref "dom.disable_window_status_change" is set, we should write the url
to the status bar even when the value of *aEventStatus is
nsEventStatus_eConsumeNoDefault. That's what it does with the patch.


> 2)  Why is this change only needed for HTML?
I don't know if this change is only needed for HTML. I tried but couldn't
reproduce the bug on svg:a.
Anyway, it's for HTML that this bug is the more annoying.

Comment 59

[Boris Zbarsky [:bzbarsky]]

> If the pref "dom.disable_window_status_change" is set, we should write the url
> to the status bar even when the value of *aEventStatus is
> nsEventStatus_eConsumeNoDefault.

Why?  Things other than user script can set the event to be consumed with no
default... Do we still want to show the text then?  That is, are we making the
(unwarranted) assumption that the only thing cancelling events is user script? 
For example, if an extension attaches a handler to an <a> that sets
window.status (which it can do no matter what the value of the "
"dom.disable_window_status_change" preference) and returns true, then we'll
ignore the fact that it returned "true" and still show the link URI instead of
the text the extension set, won't we?

To test this easily, just remove the CanSetProperty check in
nsGlobalWindow::SetStatus, apply your patch, set the pref, then see what the
behavior is for content script that sets the status on links on mouseover.

Perhaps the right solution would rather be for user script to not be able to
cancel the mouseover event if this pref is true?  That also has issues, I suspect...

> I tried but couldn't reproduce the bug on svg:a.

Here's a simple non-HTML testcase that shows the bug:

data:text/xml,<html:html xmlns:html="http://www.w3.org/1999/xhtml"><html:span
onmouseover="return true;"><a xmlns:xlink="http://www.w3.org/1999/xlink"
xlink:href="http://www.mozilla.org" xlink:type="simple">This is a link to
mozilla.org; hover it and see what happens</a></html:span></html:html>

A similar testcase with <svg:a> would show the same problem, since that uses
XLink on the inside.

> Anyway, it's for HTML that this bug is the more annoying.

This is one of those things where if we don't fix it for non-HTML, we'll just
have to go through all this again in a year or two.  I'd like to see a patch
that fixes both HTML and non-HTML at once.

Comment 60

[Hixie (not reading bugmail)]

One option is to set the status bar text before the event is fired.

Comment 61

[Boris Zbarsky [:bzbarsky]]

That would probably make the most sense, in fact....

Comment 62

[Boris Zbarsky [:bzbarsky]]

Er... Except generally speaking, if the event's preventDefault() is called we
arguably should not show the status bar text, since that is the event's default
action.

Comment 63

[Hixie (not reading bugmail)]

> Er... Except generally speaking, if the event's preventDefault() is called we
> arguably should not show the status bar text, since that is the event's default
> action.

Only be convention. We could change that easily -- most (all?) of the time if
someone is going to cancel the event to prevent us from updating the status bar,
it's because they want to do it themselves. In which case they'll do it and
overwrite our text and everything will be as expected.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196055 [details] [diff] [review]
patch v2

Fair enough.  Let's do that, then.  I like that better than the proposal here. 
Please also fix the nsXMLElement code, and in general any code that calls
OnOverLink.

Comment 65

[Brendan Eich [:brendan]]

Florian, are you willing to take assignment of this bug?

/be

Comment 66

[Florian Quèze [:florian]]

Attachment: patch v3

New patch following the idea "set the status bar text before the event is fired" (comment 60).
My patch may be wrong since I'm not sure I understand every line I moved.

Comment 67

[Boris Zbarsky [:bzbarsky]]

That patch needs review too, not just sr.

In any case, I won't be able to look at this any time soon (weeks, at least).  I'd strongly suggest finding another reviewer...

Comment 68

[Randy Aubry]

There is other odd behavoir going on, besides the mouseover behavoir that attachment (id=136498) of Comment #39 was created for. Click either link that isn't displying in the status bar, will cause it to display, BUT only upon the 1st click. Thereafter, clicks on the link will NOT display. Only after clicking upon Anything else, the body, anything, another link. ... Then once again the link will display on the 1st click.  Behavoir is better seen if href="javascript:void('')". Change status bar text has no effect on this behavoir.   This displaying upon the click is mention in 158591 which is a possible duplicate.  They did not mention this "once only" displaying.
I would think if is suppose to happen the 1st time, it should happen the 2nd and 3rd, etc. With a live link, one does not get to observe this 2nd, 3rd, etc. click behavoir.

Comment 69

[Peter Flynn (Oracle)]

(In reply to comment #68)

It's happening from the focus event, so that's why you don't see it on the second click. You have to un-focus the link and then re-focus it to see the behavior again.

You can verify this by catching onfocus on the link and doing preventDefault(). This prevents the link from getting focus, and also prevents the "first click" from displaying the URL.

Comment 70

[Darin Fisher]

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

Comment 71

[Peter Flynn (Oracle)]

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

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #66)
> Created an attachment (id=201286) [details]
> patch v3
> 
Could you perhaps update the patch, since it is from pre-event-dispatching-rewrite era.

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 201286 [details] [diff] [review]
patch v3

Clearing r?. Waiting for an updated patch.

Comment 76

[Florian Quèze [:florian]]

Attachment: patch v4

Comment 77

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 262435 [details] [diff] [review]
patch v4

For future reference, please use more context and the -p option when diffing.  That makes patches a LOT easier to review.  -up8 is a good set of options.

>Index: base/src/nsGenericElement.cpp

>+nsGenericElement::PreHandleEventForLinks(nsEventChainPreVisitor& aVisitor)

>+  if (aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault ||
>+      !NS_IS_TRUSTED_EVENT(aVisitor.mEvent) ||
>+      !aVisitor.mPresContext) {
>+    return NS_OK;
>+  }

Does that condition need to match the similar one in PostHandleEventForLinks?  If so, document so in both places.

>+  // Make sure we actually are a link before continuing

Same for this condition.  In fact, maybe this chunk of code should be factored out into a method called from both places.

>+  switch (aVisitor.mEvent->message) {

Right before that switch, I would put in a nice comment explaining why all this is being done in PreHandleEvent.

>+  case NS_MOUSE_EXIT_SYNTH:
>+    aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>+    rv = LeaveLink(aVisitor.mPresContext);
>+    break;
>+
>+  case NS_BLUR_CONTENT:
>+    rv = LeaveLink(aVisitor.mPresContext);
>+    break;

I know you just moved this, but can't you use fall-through here too?

>Index: html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::PreHandleEventForAnchors(nsEventChainPreVisitor& aVisitor)

>+  nsGenericElement::doPreHandleEvent(this, aVisitor);

Why not just call nsGenericElement::PreHandleEvent here?  I'd prefer this.  And probably bail out if that returns error.

This function copies a whole bunch of logic from nsGenericHTMLElement::PostHandleEventForAnchors.  Please factor this out into a function instead.  We really don't want those to get out of sync.

>Index: svg/content/src/nsSVGAElement.cpp

>+nsSVGAElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+  nsGenericElement::doPreHandleEvent(this, aVisitor);

Again, why not PreHandleEvent()?  And why ignore the rv?

>Index: xml/content/src/nsXMLElement.cpp

>+nsXMLElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+  nsGenericElement::doPreHandleEvent(this, aVisitor);

And here.

Comment 78

[Florian Quèze [:florian]]

Attachment: patch v5

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 262459 [details] [diff] [review]
patch v5

>Index: base/src/nsGenericElement.cpp

>+nsGenericElement::CheckHandleEventForLinksPrecondition(nsEventChainVisitor& aVisitor,

>+  if (!IsLink(aURI)) {
>+    return PR_FALSE;
>+  }
>+
>+  return PR_TRUE;

Isn't that the same thing as |return IsLink(aURI);|?

>+nsGenericElement::PreHandleEventForLinks(nsEventChainPreVisitor& aVisitor)
>+  // updated even if the event is consummed before we have a chance to set it.

"consumed".

>Index: base/src/nsGenericElement.h
>+   * Check that we meet the conditions to handle a link event
>+   * and that we are actually on a link.
>+   *
>+   * @param aVisitor event visitor
>+   * @param aURI the uri of the link [OUT]

Document what the return value means?  How is aURI related to the return value?  Can it be null if the return value is PR_TRUE?

>+   * Handle default actions for link event if the event isn't consummed yet.

"consumed"

>Index: html/content/src/nsGenericHTMLElement.cpp

>   if (target && IsArea(target) && !IsArea(this)) {
>+    return PR_FALSE;
>+  }
>+
>+  return PR_TRUE;

  return !target || !IsArea(target) || IsArea(this);

>+nsGenericHTMLElement::PreHandleEventForAnchors(nsEventChainPreVisitor& aVisitor)

>+  if (rv != NS_OK) {
>+    return rv;
>   }

  NS_ENSURE_SUCCESS(rv, rv);

please.

>+  if (!CheckHandleEventForAnchorsPreconditions(aVisitor))
>+    return NS_OK;

Please put curlies around the body of the if.

>+nsGenericHTMLElement::PostHandleEventForAnchors(nsEventChainPostVisitor& aVisitor)

>+  if (!CheckHandleEventForAnchorsPreconditions(aVisitor))
>+    return NS_OK;

Same here.

>Index: html/content/src/nsGenericHTMLElement.h
>+  PRBool CheckHandleEventForAnchorsPreconditions(nsEventChainVisitor& aVisitor);

Document what this function does?  Specifically, what does a PR_TRUE or PR_FALSE return mean?

>Index: svg/content/src/nsSVGAElement.cpp
>+nsSVGAElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+  if (rv != NS_OK) {
>+    return rv;
>+  }

  NS_ENSURE_SUCCESS(rv, rv);

>Index: xml/content/src/nsXMLElement.cpp
>+nsXMLElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+  if (rv != NS_OK) {
>+    return rv;
>+  }

NS_ENSURE_SUCCESS(rv, rv);

sr=bzbarsky with those nits picked.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 262459 [details] [diff] [review]
patch v5

Fix bz's comments, r=me

Comment 81

[Florian Quèze [:florian]]

Attachment: patch v6

Should be ready for checkin.

Comment 82

[Florian Quèze [:florian]]

Attachment: patch v6 bis

Comment 84

[bgg0]

actually no idea about status bar but you can find some useful status at
http://whatsappstatushub.in/friendship-status/
http://whatsappstatushub.in/friendship-status-in-hindi/
http://whatsappstatushub.in/