345521 - "return 0" in handler cancels events

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

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...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hixie, is this documented in HTML5 yet?

Comment 4

[Hixie (not reading bugmail)]

Yeah, this is documented in a first-draft form:
   http://whatwg.org/specs/web-apps/current-work/#event

Comment 5

[Boris Zbarsky [:bzbarsky]]

That just talks about returning false.  It doesn't make it clear that things that are "equivalent" to false in ECMAScript don't count...

Comment 6

[Hixie (not reading bugmail)]

Like I said, first-draft form. Send comments to [e-mail] 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).

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Patch to fix and test event cancel behaviour

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.

Comment 8

[Boris Zbarsky [: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?  ;)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

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

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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....

Comment 12

[Dave Liebreich [:davel]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Dave Liebreich [:davel]]

[added 347708 as the meta bug for tracking tests to be added to suites]

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 16

[Dave Liebreich [:davel]]

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.

Comment 17

[Robert Sayre]

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