In bug 74688, we hacked around not being able to prevent the default action via XBL handler JS with JS turned off. The real fix, after a discussion with hyatt is to add an attribute preventdefault="true".

I've got this working in my tree. I should have a patch up here soon.

Comment on attachment 87630 [details] [diff] [review] Proposed fix v1.0 > void > nsXBLPrototypeHandler::ConstructPrototype(nsIContent* aKeyElement, > const PRUnichar* aEvent, const PRUnichar* aPhase, > const PRUnichar* aAction, const PRUnichar* aCommand, > const PRUnichar* aKeyCode, const PRUnichar* aCharCode, > const PRUnichar* aModifiers, const PRUnichar* aButton, >- const PRUnichar* aClickCount) >+ const PRUnichar* aClickCount, const PRUnichar* aPreventDefault) > { >+ mType = nsnull; Just a nit: use |0| instead of |nsnull| for a PRUint8. > >+ nsAutoString preventDefault(aPreventDefault); >+ if (!preventDefault.IsEmpty()) { >+ if (preventDefault.Equals(NS_LITERAL_STRING("true"))) { >+ mType |= NS_HANDLER_TYPE_PREVENTDEFAULT; >+ } >+ } > } Get jag to double-check this... I'm not sure the IsEmpty() check is needed here. It may be slightly faster to just use nsCRT::strcmp, since wrapping aPreventDefault with an nsAutoString is O(n) (n=string length) since it has to calculate the length. nsCRT::strcmp will also stop right away if aPreventDefault is null. r=bryner.

Changes: - Fix bryner's nit about mType = 0; - Returning NS_OK if preventdefault="true" but we have no mHandlerElement instead of NS_ERROR_FAILURE in ExecuteHandler(). - Added a comment explaining this, and a few other minor comment changes. I left the string issue alone for jag clarify. Note that everything else in that function uses IsEmpty(), so it might be a good cleanup to get in. I'll get jag to sr= this.

Comment on attachment 88158 [details] [diff] [review] Patch v.1.1 r=bryner

Comment on attachment 88158 [details] [diff] [review] Patch v.1.1 >Index: content/xbl/src/nsXBLPrototypeHandler.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v >retrieving revision 1.55 >diff -u -r1.55 nsXBLPrototypeHandler.cpp >--- content/xbl/src/nsXBLPrototypeHandler.cpp 15 May 2002 18:53:05 -0000 1.55 >+++ content/xbl/src/nsXBLPrototypeHandler.cpp 18 Jun 2002 19:15:00 -0000 >@@ -126,8 +126,8 @@ > > nsXBLPrototypeHandler::~nsXBLPrototypeHandler() > { >- gRefCnt--; >- if (mType != NS_HANDLER_TYPE_XUL && mHandlerText) >+ --gRefCnt; >+ if ((NS_HANDLER_TYPE_XUL & ~mType) && mHandlerText) How about !(mType & NS_HANDLER_TYPE_XUL) or ~mType & NS_HANDLER_TYPE_XUL to be more in line with the rest of the checks (mType left, flag right)? >@@ -207,11 +207,22 @@ > nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver* aReceiver, > nsIDOMEvent* aEvent) > { >+ nsresult rv = NS_ERROR_FAILURE; >+ >+ // Check if we're preventing default first since both >+ // mHandlerElement and mHandlerText will not be set >+ // and hence both be null if we are getting called from >+ // <handler action="foo" preventdefault="true"/> >+ if (mType & NS_HANDLER_TYPE_PREVENTDEFAULT) { >+ aEvent->PreventDefault(); >+ rv = NS_OK; >+ } >+ > if (!mHandlerElement) // This works for both types of handlers. In both cases, the union's var should be defined. >- return NS_ERROR_FAILURE; >+ return rv; I don't think your comment above should focus on that one case. You could have something like <handler action="foo" preventdefault="true">doStuff();</handler>. In either case you just want to call aEvent->PreventDefault(); The fact that you have to check it before checking mHandlerElement should be obvious from the early return. You could add a comment to |rv = NS_OK;| though, e.g. nsresult rv = NS_ERROR_FAILURE; // Prevent default action? if (mType & NS_HANDLER_TYPE_PREVENTDEFAULT) { aEvent->PreventDefault(); // if we prevent default it's okay for mHandlerElement and // mHandlerText to be null rv = NS_OK; } if (!mHandlerElement) // This works for both types of handlers. In both cases, the union's var should be defined. return rv; >@@ -923,13 +933,19 @@ > } > else { > key.Assign(aKeyCode); >- if (mType == NS_HANDLER_TYPE_XUL) >+ if (mType & NS_HANDLER_TYPE_XUL) > mHandlerElement->GetAttr(kNameSpaceID_None, nsXBLAtoms::keycode, key); > > if (!key.IsEmpty()) > mDetail = GetMatchingKeyCode(key); > } > >+ nsAutoString preventDefault(aPreventDefault); >+ if (!preventDefault.IsEmpty()) { >+ if (preventDefault.Equals(NS_LITERAL_STRING("true"))) { >+ mType |= NS_HANDLER_TYPE_PREVENTDEFAULT; >+ } >+ } This IsEmpty() check isn't needed. You can always use .Equals, and an empty string won't be equal to "true". sr=jag

Checked in.