242234 - support onclick type attributes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

Comment 2

[Hixie (not reading bugmail)]

This is where we run into the evt/event issue I guess.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes. It's probably not a big problem implementation wise, the bigger issue is
going to be webauthors.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

tor pointed out that the argument name 'event' is decided in the function
nsJSContext::CompileEventHandler. Specifically in the |gEventArgv| variable.
Unfortunatly that function doesn't know which element (or namespace thereof) the
eventhandler is sitting on, so we'll probably have to add another argument

Comment 5

[tor]

Attachment: merge to tip, use "evt"

Comment 6

[Hixie (not reading bugmail)]

(do we have to hardcode the event name in two places?)

Comment 7

[tor]

Orginally I was thinking of passing the namespace down to CompileEventHandler,
but jst didn't want it knowing about namespaces and suggested passing the
character string around.  I'll see what he thinks of using a aIsSVG flag instead.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm fine with the current way I think, though maybe I'd prefer to pass an atom.
Of course, I'd be fine with a bool too (which could be expanded to an enum in
case some another WG smokes a little too much weed too).

Comment 9

[tor]

Attachment: clean up evt/event

Comment 10

[tor]

There are two problems with this patch, which probably be addressed as
followup bugs/patches.

  * Some event names will be wrong - mozilla's event code generates html
    events (ex. "focus") instead of the DOM names (ex. "DOMfocus").  Fixing
    this would involve changing the event manager and probably some other
    code.

  * Only one "onload" is allowed, as it gets hooked up to the window object.
    This is a problem shared with html.

Comment 11

[tor]

Attachment: onload only on root

As a compromise that hopefully will provide the least surprise until the
matter of multiple onloads is addressed, this patch will only allow "onload"
on the root node.  While this won't help mixed namespace documents, it
should be useful for initialization in standalone svg files.

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 152529 [details] [diff] [review]
onload only on root

Looks ok to me; i'd suggest having jst sr=

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 152529 [details] [diff] [review]
onload only on root

+nsContentUtils::GetEventArgName(PRInt32 aNameSpaceID)
+{
+  if (aNameSpaceID == kNameSpaceID_SVG)
+    return gSVGEventName;
+  else
+    return gEventName;
+}

I'd loose the else-after-return there :-)

sr=jst

Comment 14

[tor]

Checked in.  Bug 252631 filed about the one onload per document limit.

Comment 15

[Alex Fritze]

Slight problem with xbl handlers:

If you now attach a binding to an element in the SVG namespace and the binding
contains a handler, then the handler's event parameter is undefined (it is now
called 'evt'). I guess that's a bad thing; xbl handler event args should always
be called 'event' as given by the xbl specs.
Testcases:
http://www.croczilla.com/svg/samples/xbl1/ : not working anymore because event
is not defined in xbl1-bindings.xml line 72

http://www.croczilla.com/svg/samples/xbl-shapes/ : still working, since the
elements bound are not in the svg namespace

Comment 16

[Alex Fritze]

Another problem:
onload doesn't appear to be working.
Testcase: http://www.croczilla.com/~alex/testcases/onload.svg

Comment 17

[tor]

Attachment: repair onload

The onload problem was caused by two things:

  * onload wasn't being accepted as an event name

  * check for root element was going wrong because <svg> is parsed before
    being added to the document

Jonas, why did you seperate off most of the event attributes into
IsGraphicElementEventName?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 154119 [details] [diff] [review]
repair onload

>Index: nsSVGElement.cpp
>-          mDocument->GetRootContent() == NS_STATIC_CAST(nsIContent*, this)) {
>+          mDocument->GetRootContent() == NULL) {

Ack, this is a big hack. This will no longer enforce the root-element
requirement, but rather some empty-document requirement. Which in theory could
be any element if a DOM is created in javascript. I think it's ok for now
though since onload will be changed anyway later on. Please add a comment
though.

The reason i had a separate IsGraphicElementEventName is that many elements
will have only those eventattributes. My idea was that different elements will
override IsEventName and all "graphic elements" will in their implementation
call IsGraphicElementEventName.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> mDocument->GetRootContent() == NULL

yay, scc and jst would slap you hard for that. Make that
!mDocument->GetRootContent()

Comment 20

[tor]

Onload fix checked in with the test changed and the comment expanded.

Comment 21

[tor]

Attachment: always compile xbl bindings with XBL namespace event name

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 154141 [details] [diff] [review]
always compile xbl bindings with XBL namespace event name

sr=jst

Comment 23

[tor]

XBL binding patch checked in.