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

RESOLVED FIXED in mozilla1.9alpha4

Status

()

Core
Event Handling
P3
normal
RESOLVED FIXED
18 years ago
6 years ago

People

(Reporter: Christos Cheretakis, Assigned: florian)

Tracking

(Depends on: 2 bugs, {testcase})

Trunk
mozilla1.9alpha4
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -
blocking-aviary1.5 -
blocking1.9 ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 6 obsolete attachments)

(Reporter)

Description

18 years ago
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
(Reporter)

Comment 1

18 years ago
Created attachment 9249 [details]
saved test-case, should use images, I suppose

Comment 2

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

Comment 3

18 years ago
Created attachment 9271 [details]
same testcase, with <base href...> so that images show up

Comment 4

18 years ago
Created attachment 9272 [details]
simpler testcase

Comment 5

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

related: bug 38380 (another onmouseover="return true;" bug)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Linux → All
Hardware: PC → All
Summary: onmouseover not showing target url for one link → onmouseover="return true;" --> status bar doesn't show url for links

Comment 6

17 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 7

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

17 years ago
>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?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 9

17 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

17 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

17 years ago
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?
(Reporter)

Comment 12

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

17 years ago
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.
Target Milestone: --- → Future

Comment 14

17 years ago
Mass update:  changing qacontact to ckritzer@netscape.com
QA Contact: janc → ckritzer

Comment 15

17 years ago
Updating QA Contact.
QA Contact: ckritzer → lorca

Comment 16

17 years ago
Created attachment 19382 [details]
Testcase with text along with onmouseover in image

Comment 17

17 years ago
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) 
Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok

Comment 19

16 years ago
QA contact updated
QA Contact: gerardok → madhur

Comment 20

16 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → WORKSFORME

Updated

16 years ago
QA Contact: madhur → rakeshmishra

Comment 21

16 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 22

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

Comment 23

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

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

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

Comment 26

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

Updated

15 years ago
QA Contact: rakeshmishra → trix

Comment 27

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

Comment 28

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

Comment 29

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

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

15 years ago
*** Bug 190609 has been marked as a duplicate of this bug. ***
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

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

Comment 34

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

14 years ago
To clarify what pi said, this bug affects Alltheweb *if you have disallowed
scripts changing window.status*.
Summary: onmouseover="return true;" --> status bar doesn't show url for links → disallowing "change status bar text" often leaves status bar empty for links (onmouseover="return true;" --> status bar doesn't show url for links)

Comment 36

14 years ago
.
Assignee: joki → saari
Status: REOPENED → NEW
QA Contact: trix → ian

Comment 37

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

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

14 years ago
Created attachment 136498 [details]
Different link behaviours, even after prefs.js setting

Comment 40

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

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

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

Comment 43

14 years ago
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?
Assignee: saari → events

Comment 44

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

Updated

13 years ago
Flags: blocking-aviary1.0?
*** Bug 266034 has been marked as a duplicate of this bug. ***

Comment 46

13 years ago

(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.
*** Bug 267337 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Flags: blocking-aviary1.0? → blocking-aviary1.0-

Comment 48

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

Updated

13 years ago
Flags: blocking-aviary1.1?

Comment 49

13 years ago
"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

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

Comment 51

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

Comment 52

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

Updated

13 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-

Comment 53

12 years ago
*** Bug 299765 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 54

12 years ago
Created attachment 195783 [details] [diff] [review]
patch v1
(Assignee)

Updated

12 years ago
Attachment #195783 - Flags: review?(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.
Attachment #195783 - Flags: review?(jst) → review+
(Assignee)

Comment 56

12 years ago
Created attachment 196055 [details] [diff] [review]
patch v2
Attachment #195783 - Attachment is obsolete: true
Attachment #196055 - Flags: superreview?(bzbarsky)
Attachment #196055 - Flags: review+
So:

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

Comment 58

12 years ago
(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.
> 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.
One option is to set the status bar text before the event is fired.
That would probably make the most sense, in fact....
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.
> 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 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.
Attachment #196055 - Flags: superreview?(bzbarsky) → superreview-
Florian, are you willing to take assignment of this bug?

/be
(Assignee)

Comment 66

12 years ago
Created attachment 201286 [details] [diff] [review]
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.
Assignee: events → f.qu
Attachment #196055 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #201286 - Flags: superreview?(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...
(Assignee)

Updated

12 years ago
Attachment #201286 - Flags: superreview?(peterv)
Attachment #201286 - Flags: superreview?(bzbarsky)
Attachment #201286 - Flags: review?(jst)

Comment 68

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

12 years ago
(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

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

Comment 71

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

Updated

11 years ago
Duplicate of this bug: 372968

Updated

11 years ago
Flags: blocking1.9?

Updated

11 years ago
Duplicate of this bug: 375927
Attachment #201286 - Flags: review?(jst) → review?(Olli.Pettay)

Comment 74

11 years ago
(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

11 years ago
Comment on attachment 201286 [details] [diff] [review]
patch v3

Clearing r?. Waiting for an updated patch.
Attachment #201286 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 76

11 years ago
Created attachment 262435 [details] [diff] [review]
patch v4
Attachment #201286 - Attachment is obsolete: true
Attachment #262435 - Flags: superreview?(bzbarsky)
Attachment #262435 - Flags: review?(Olli.Pettay)
Attachment #201286 - Flags: superreview?(peterv)
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.
Attachment #262435 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 78

11 years ago
Created attachment 262459 [details] [diff] [review]
patch v5
Attachment #262435 - Attachment is obsolete: true
Attachment #262459 - Flags: superreview?(bzbarsky)
Attachment #262459 - Flags: review?(Olli.Pettay)
Attachment #262435 - Flags: review?(Olli.Pettay)
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.
Attachment #262459 - Flags: superreview?(bzbarsky) → superreview+

Comment 80

11 years ago
Comment on attachment 262459 [details] [diff] [review]
patch v5

Fix bz's comments, r=me
Attachment #262459 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 81

11 years ago
Created attachment 262474 [details] [diff] [review]
patch v6

Should be ready for checkin.
Attachment #262459 - Attachment is obsolete: true
(Assignee)

Comment 82

11 years ago
Created attachment 262475 [details] [diff] [review]
patch v6 bis
Attachment #262474 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9alpha4
Duplicate of this bug: 389995
Depends on: 440040

Updated

6 years ago
Depends on: 731106
You need to log in before you can comment on or make changes to this bug.