811753 - Add nsIContent::IsEventAttributeName

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we have various places where we call nsContentUtils::IsEventAttributeName with various flags, and those flags depend on the element we're dealing with.  We should just have a virtual nsIContent::IsEventAttributeName which will call nsContentUtils with the right flags for that element.

Comment 1

[Julien Levesy]

Attachment: Need feedback on this patch for validating my approach. Do not try to compile.

Need feedback on this patch for validating my approach. Do not try to compile.

My main problem is that i don't know class model very well, and this is a huge model i think. So it's difficult to me to determine where i have to implement the IsEventAttributeName method.
I don't want to make useless code duplications.
I'll try to do my best to deal with that !

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695260 [details]
Need feedback on this patch for validating my approach. Do not try to compile.

So I think the right thing is to have IsEventAttributeName on nsIContent not be pure virtual and just return false.

You'll then need overrides on nsGenericHTMLElement (which all HTML elements inherit from), nsXULElement, nsSVGElement (which all SVG elements inherit from), and probably nsHTMLBodyElement and nsHTMLFrameSetElement because some event attribute names are only supported on those.

Comments and text nodes and document fragments and document types and so forth can't have attributes, so always returning false for those is fine.

Thank you for working on this!

Comment 3

[Julien Levesy]

Ok. Thanks for your complete answer.
I'll propose a patch soon !

Comment 4

[Julien Levesy]

Attachment: bug-811753-fix proposition

Here it is. 
I also changed calls in nsXMLContentSerializer::IsJavaScript method.
Is there any test case to write ? execute ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695309 [details] [diff] [review]
bug-811753-fix proposition

Sorry for the lag here, holidays...

This is a patch on top of the first patch in this bug, right?  At some point we'll need a combined patch against current tip for check-in.

>+   * @note base behaviour return false, it is overrided 

"overridden by subclasses as needed"?

nssXMLContentSerializer::IsJavaScript needs to return the return value of IsEventAttributeName.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_HTML);

Space after comma, please.

>+++ b/content/html/content/src/nsGenericHTMLElement.h

I don't think you need to copy/paste the API documentation comment here, especially since it's not actually true as written for this class.  Just remove the comment, please.  Same for the other headers where this comment ended up.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_HTML |
>+                                                EventNameType_HTMLBodyOrFramesetOnly);

Space after comma, and fix the indent please, in both body and frameset code.

You probably want to have an SVG peer look at the nsSVGElement change now that I look at it more closely.  In particular, SVG elements already have an IsEventName function that does what we want, I think; it might be enough to rename it.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_XUL);

Space after comma, please.

You can simplify the code in nsGenericHTMLElement::AfterSetAttr to use this new function, I believe.

r=me with the nits above picked, on all but the SVG bit.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 695309 [details] [diff] [review]
bug-811753-fix proposition

>+
>+bool
>+nsSVGElement::IsEventAttributeName(nsIAtom *aName)
>+{
>+  return nsContentUtils::IsEventAttributeName(aName,
>+    EventNameType_SVGGraphic |
>+    EventNameType_SVGSVG |
>+    EventNameType_SMIL);
>+}

Here's the table of what goes where for SVG: http://www.w3.org/TR/SVG/attindex.html check out the onxxx attributes.

EventNameType_SMIL should go on animation elements, which it does in http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGAnimationElement.cpp#496 but it shouldn't go on other SVG elements which means the flag should come out of nsSVGElement::IsEventAttributeName

EventNameType_SVGGraphic should go on some but not all SVG elements (again, it does in various places e.g. http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#858) so that should not be here either.

Which leaves EventNameType_SVGSVG which occurs only on the <svg> element of all the SVG elements so that should be set only in nsSVGSVGElement::IsEventAttributeName, and again that is so.

So in conclusion we don't want any of these flags set with nsSVGElement::IsEventAttributeName. Which means, we don't want the method at all here.

r=longsonr with nsSVGElement::IsEventAttributeName removed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, it needs to be replaced by IsEventAtributeName overloads on other SVG elements, right?

Comment 8

[Robert Longson [:longsonr]]

Such things already exist.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Such things already exist.

Where?

Comment 10

[Robert Longson [:longsonr]]

Ahh you're right of course, all the SVG instances need to be renamed from IsEventAttributeName to IsEventName . Basically everything in here:

http://mxr.mozilla.org/mozilla-central/search?string=IsEventAttributeName&find=content/svg

Preferably with a MOZ_OVERRIDE modifier post override too.

Comment 11

[Julien Levesy]

Thanks for answers.

Sorry about "stupid" mistakes(spaces after comma, or duplicated comments...)

Robert, you mean from IsEventName to IsEventAttributeName ?  (to enable polymorphism on those classes ?) and remove the nsSVGElement:IsEventName hook ? 

I'll put MOZ_OVERRIDE modifier on all overriden methods. 

Thanks

Comment 12

[Robert Longson [:longsonr]]

(In reply to Julien Levesy from comment #11)
> 
> Robert, you mean from IsEventName to IsEventAttributeName ?  (to enable
> polymorphism on those classes ?) and remove the nsSVGElement:IsEventName
> hook ? 
> 

Yes to both.

Comment 13

[Julien Levesy]

Attachment: Bug 811753 patch proposal, r2.

Here is a patch proposal.
I ran local test suite, it leads on an unexpected failure.

/tests/Harness_sanity/test_sanityEventUtils.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0

I'm sorry but i need a little help here.
Thanks !

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 696892 [details] [diff] [review]
Bug 811753 patch proposal, r2.

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
>--- a/content/html/content/src/nsGenericHTMLElement.h
>+++ b/content/html/content/src/nsGenericHTMLElement.h
>@@ -1,9 +1,9 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+

I think this is a mistake. Won't help you pass the test though unfortunately.

Comment 15

[Julien Levesy]

Attachment: Bug 811753 patch proposal, r2.

I accidentaly removed a header line on the previous patch.
Sorry.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I ran local test suite, it leads on an unexpected failure.

Hmm.  The only event attribute in there is the onload="starttest()" on the <body>, which is clearly getting called.

That said, the test as written looks bogus.  The "SimpleTest.waitForExplicitFinish();" should be the first line of starttest(), not the first line of the waitForFocus callback.  Does doing that fix things for you locally?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696894 [details] [diff] [review]
Bug 811753 patch proposal, r2.

>+++ b/content/base/public/nsIContent.h
>+    * a given element type. Types are from the EventNameType enumeration
>+    * defined above.

Please drop the sentence starting with "Types", since it's nonsensical here.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>@@ -797,17 +797,17 @@ nsGenericHTMLElement::AfterSetAttr(int32

>     uint32_t eventType = EventNameType_HTML;
>     if (mNodeInfo->Equals(nsGkAtoms::body) ||
>         mNodeInfo->Equals(nsGkAtoms::frameset)) {
>       eventType |= EventNameType_HTMLBodyOrFramesetOnly;
>     }

All of that calculation of eventType is now dead code and can go away, right?

>+    if (IsEventAttributeName(aName) &&
>         aValue) {

Please pull up aValue to the same line as the first part of the condition now that it fits.

r=me with that.  Thank you again!

I'll push this patch to try to see what tests look like.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9bc03b452959

Comment 19

[Julien Levesy]

Attachment: Bug 811753 patch proposal, r3.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 697142 [details] [diff] [review]
Bug 811753 patch proposal, r3.

r=me

Thank you again for the patch!  Try looks green; I'll push this.

Comment 21

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dd897bfc2469

Julien, thanks for sticking with this!

Comment 22

[Julien Levesy]

You're welcome !
Thanks both for help !

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dd897bfc2469