Closed
Bug 40838
Opened 24 years ago
Closed 18 years ago
disallowing "change status bar text" often leaves status bar empty for links (onmouseover="return true;" --> status bar doesn't show url for links)
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P3)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha4
People
(Reporter: xalkina, Assigned: florian)
References
(Depends on 2 open bugs, )
Details
(Keywords: testcase)
Attachments
(6 files, 6 obsolete files)
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•24 years ago
|
||
Comment 2•24 years ago
|
||
fixing url so it isn't a porn site :)
Comment 3•24 years ago
|
||
Comment 4•24 years ago
|
||
Comment 5•24 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•24 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
Closed: 24 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 7•24 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•24 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•24 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
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 10•24 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•24 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•24 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•24 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•24 years ago
|
||
Mass update: changing qacontact to ckritzer@netscape.com
QA Contact: janc → ckritzer
Comment 16•24 years ago
|
||
Comment 17•24 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)
Comment 18•24 years ago
|
||
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 20•23 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
Closed: 24 years ago → 23 years ago
Resolution: --- → WORKSFORME
Updated•23 years ago
|
QA Contact: madhur → rakeshmishra
Comment 21•23 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•23 years ago
|
||
I confirm (today's build of the 1.0.0 branch under Linux)
Comment 23•22 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•22 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•22 years ago
|
||
*** Bug 166923 has been marked as a duplicate of this bug. ***
Comment 26•22 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•22 years ago
|
QA Contact: rakeshmishra → trix
Comment 27•22 years ago
|
||
*** Bug 135849 has been marked as a duplicate of this bug. ***
Comment 28•22 years ago
|
||
*** Bug 177928 has been marked as a duplicate of this bug. ***
Comment 29•22 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•22 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•22 years ago
|
||
*** Bug 190609 has been marked as a duplicate of this bug. ***
Comment 32•22 years ago
|
||
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•22 years ago
|
||
*** Bug 203970 has been marked as a duplicate of this bug. ***
Comment 34•21 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•21 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 37•21 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•21 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•21 years ago
|
||
Comment 40•21 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•21 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•21 years ago
|
||
*** Bug 227159 has been marked as a duplicate of this bug. ***
Comment 43•21 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?
Updated•21 years ago
|
Assignee: saari → events
Comment 44•20 years ago
|
||
*** Bug 251166 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.0?
Comment 45•20 years ago
|
||
*** Bug 266034 has been marked as a duplicate of this bug. ***
Comment 46•20 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.
Comment 47•20 years ago
|
||
*** Bug 267337 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Comment 48•20 years ago
|
||
*** Bug 268873 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.1?
Comment 49•20 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•20 years ago
|
||
*** Bug 280929 has been marked as a duplicate of this bug. ***
Comment 51•20 years ago
|
||
*** Bug 287500 has been marked as a duplicate of this bug. ***
Comment 52•20 years ago
|
||
*** Bug 289354 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Comment 53•19 years ago
|
||
*** Bug 299765 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 54•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #195783 -
Flags: review?(jst)
Comment 55•19 years ago
|
||
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•19 years ago
|
||
Attachment #195783 -
Attachment is obsolete: true
Attachment #196055 -
Flags: superreview?(bzbarsky)
Attachment #196055 -
Flags: review+
Comment 57•19 years ago
|
||
So:
1) Why does this work?
2) Why is this change only needed for HTML?
Assignee | ||
Comment 58•19 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.
Comment 59•19 years ago
|
||
> 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•19 years ago
|
||
One option is to set the status bar text before the event is fired.
Comment 61•19 years ago
|
||
That would probably make the most sense, in fact....
Comment 62•19 years ago
|
||
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•19 years ago
|
||
> 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•19 years ago
|
||
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-
Comment 65•19 years ago
|
||
Florian, are you willing to take assignment of this bug?
/be
Assignee | ||
Comment 66•19 years ago
|
||
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)
Comment 67•19 years ago
|
||
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•19 years ago
|
Attachment #201286 -
Flags: superreview?(peterv)
Attachment #201286 -
Flags: superreview?(bzbarsky)
Attachment #201286 -
Flags: review?(jst)
Comment 68•19 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•19 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•19 years ago
|
||
*** Bug 333793 has been marked as a duplicate of this bug. ***
Comment 71•19 years ago
|
||
*** Bug 158591 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Flags: blocking1.9?
Attachment #201286 -
Flags: review?(jst) → review?(Olli.Pettay)
Comment 74•18 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•18 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•18 years ago
|
||
Attachment #201286 -
Attachment is obsolete: true
Attachment #262435 -
Flags: superreview?(bzbarsky)
Attachment #262435 -
Flags: review?(Olli.Pettay)
Attachment #201286 -
Flags: superreview?(peterv)
Comment 77•18 years ago
|
||
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•18 years ago
|
||
Attachment #262435 -
Attachment is obsolete: true
Attachment #262459 -
Flags: superreview?(bzbarsky)
Attachment #262459 -
Flags: review?(Olli.Pettay)
Attachment #262435 -
Flags: review?(Olli.Pettay)
Comment 79•18 years ago
|
||
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•18 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•18 years ago
|
||
Should be ready for checkin.
Attachment #262459 -
Attachment is obsolete: true
Assignee | ||
Comment 82•18 years ago
|
||
Attachment #262474 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 18 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9alpha4
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•