Closed Bug 345521 Opened 14 years ago Closed 14 years ago

"return 0" in handler cancels events

Categories

(Core :: DOM: Events, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: stephend, Assigned: markh)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

Summary: Same-domain, relative-pathed link fails to load.

Not sure if this belongs in HTML: Parser.  Punt as needed...

Build ID: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060721 Minefield/3.0a1

Steps to Reproduce:
1. Load http://netforbeginners.about.com/od/peersharing/ss/playingbinfiles.htm
2. Click on any of steps 2-7

Expected Results:
Link loads

Actual Results:
Link fails to load
So, I created a replica of the page and set its <base href="http://netforbeginners.about.com/od/peersharing/ss"> and did NOT see the problem...
This is a DOM_AGNOSTIC regression.  The relevant change in nsJSEventListener::HandleEvent is replacing 

  } else if (JSVAL_IS_BOOLEAN(rval)) { 

with

               dataType == nsIDataType::VTYPE_BOOL ||                           
               dataType == nsIDataType::VTYPE_INT8 ||                           
               dataType == nsIDataType::VTYPE_INT16 ||                          
               dataType == nsIDataType::VTYPE_INT32 ||                          
               dataType == nsIDataType::VTYPE_UINT8 ||                          
               dataType == nsIDataType::VTYPE_UINT16 ||                         
               dataType == nsIDataType::VTYPE_UINT32)

which is NOT the same thing at all.  We should just be checking for BOOL, probably.   But check what the variant comes from, I guess....

Hixie, is this documented in HTML5 yet?

Over to Mark.  Please make sure to check in a regression test for this when you check in the fix.
Assignee: mrbkap → mhammond
Blocks: dom-agnostic
Component: HTML: Parser → DOM: Events
Flags: blocking1.9a1+
Keywords: regression
OS: Windows XP → All
Hardware: PC → All
Summary: Same-domain, relative-pathed link fails to load. → "return 0" in handler cancels events
Hixie, is this documented in HTML5 yet?
QA Contact: parser → ian
Yeah, this is documented in a first-draft form:
   http://whatwg.org/specs/web-apps/current-work/#event
That just talks about returning false.  It doesn't make it clear that things that are "equivalent" to false in ECMAScript don't count...
Like I said, first-draft form. Send comments to whatwg@whatwg.org and I'll fix the spec. I'm not sure what should happen; in general here I'd say we should be compatible with IE (which I believe doesn't do type-conversion here, but I could be wrong).
Here is a patch that fixes this behaviour, and a simple test for the existing dom/tests/html/jshandlers.html file.

I can also confirm that IE behaves the same as Mozilla intends - ie, after applying this patch, IE and Moz again behave the same.
Attachment #232080 - Flags: superreview?(bzbarsky)
Attachment #232080 - Flags: review?(bzbarsky)
Comment on attachment 232080 [details] [diff] [review]
Patch to fix and test event cancel behaviour

r+sr=bzbarsky.  Does anyone ever run that test?  ;)
Attachment #232080 - Flags: superreview?(bzbarsky)
Attachment #232080 - Flags: superreview+
Attachment #232080 - Flags: review?(bzbarsky)
Attachment #232080 - Flags: review+
Checking in src/events/nsJSEventListener.cpp;
new revision: 1.58; previous revision: 1.57
Checking in tests/html/jshandlers.html;
new revision: 1.2; previous revision: 1.1
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified FIXED using build 2006-08-06-07 of SeaMonkey trunk under Windows XP.  I checked both my original live testcase as well as the data URL bz provided.
Status: RESOLVED → VERIFIED
Would it be possible to add a test for this to one of our test suites?  I'm not sure whether we can do this in the context of the content unit tests, say....
We should have a test suite running soon, so here is the javascript needed to run this test using jssh:

w=getWindows()[0];
w.content.location='data:text/html,<a href="http://www.mozilla.org" onclick="return 0">Click me</a>';
d=w.content.document;
var evt = d.createEvent("MouseEvents");
evt.initMouseEvent("click", true, true, w,
  0, 0, 0, 0, 0, false, false, false, false, 0, null);
d.links.item(0).dispatchEvent(evt);
assertEquals("click functions when onclick returns 0","http://www.mozilla.org", w.content.location); // jsUnit-style test
davel, is there a way to flag this bug so we remember to check in the test once  the test suite is up?  Or should I just file a followup bug for doing that?
[added 347708 as the meta bug for tracking tests to be added to suites]
(In reply to comment #13)
> davel, is there a way to flag this bug so we remember to check in the test once
>  the test suite is up?  Or should I just file a followup bug for doing that?

I thought that the |in-testsuite?| flag was meant to be used for that, and then changed to "in-testsuite+" once the test is checked in, or "in-testsuite-" if no test is needed.
Flags: in-testsuite?
I'd rather use the tracking bug, at least until the test suites exist.  Please note that once the test suites exist, the responsibility for adding tests falls on the developers.
RCS file: /cvsroot/mozilla/testing/mochitest/tests/test_bug345521.html,v
done
Checking in tests/test_bug345521.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug345521.html,v  <--  test_bug345521.html
initial revision: 1.1
done
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.