366478 - consolidate lists of onX event attributes

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Neil Deakin]

Attachment: consolidate event lists

- so that XUL can use the additional ones like onscroll, onDOMFocusIn, etc
- don't include overflowchanged and paint events, neither of which do anything

There's also some additional lists of event names which could also be consolidated in DOMClassInfo::ReallyIsEventName and nsHTMLContentSerializer::IsJavaScript

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 250987 [details] [diff] [review]
consolidate event lists 

>+enum EventNameType {
>+  EventNameType_HTML = 0x0001,
>+  EventNameType_XUL = 0x0002,
>+  EventNameType_SVGGraphic = 0x0004, // svg graphic elements
>+  EventNameType_SVGSVG = 0x0008, // the svg element
>+
>+  EventNameType_HTMLXUL = 0x0003,

I don't quite like EventNameType_HTMLXUL, but it seems to be useful, so ok to me.

+struct EventNameMapping {
+  PRUint32  mId;
+  EventNameType mType;
+};

Should mType be PRUint32, because you're actually assigning it values which aren't
in the EventNameType enum. Then you could remove the (c-style) casts in
nsContentUtils::InitializeEventTable.

> +   * Determines if an event is valid for a given element type.
> +   * 
> +   */
> +  static PRBool IsEventName(nsIAtom* aName, EventNameType aType);
> +

The comment and also the method name should be a bit different.
Maybe IsEventAttributeName(...)


>-  const char* name;
>-  aName->GetUTF8String(&name);
>-
>-  if (name[0] != 'o' || name[1] != 'n') {
>-    return PR_FALSE;
>-  }

I wonder if this optimization is still needed. Possibly not, but when landing keep
eye on Tp.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

> 
> > +   * Determines if an event is valid for a given element type.
> > +   * 
> > +   */
> > +  static PRBool IsEventName(nsIAtom* aName, EventNameType aType);
> > +
> 
> The comment and also the method name should be a bit different.
> Maybe IsEventAttributeName(...)
> 

Ah, nm, because this is used also in ELM, the method name is ok.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> 
> Ah, nm, because this is used also in ELM, the method name is ok.
> 
And I should read the patch before adding comments :) No, it is not used in ELM, only GetEventId() is used there. So I prefer IsEventAttributeName.

Comment 4

[Neil Deakin]

Attachment: fix issues described above

Comment 5

[Neil Deakin]

Comment on attachment 252786 [details] [diff] [review]
fix issues described above

need a review on the svg parts here too

Comment 6

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

Comment on attachment 252786 [details] [diff] [review]
fix issues described above

>Index: content/base/public/nsContentUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsContentUtils.h,v
>retrieving revision 1.115
>diff -u -p -8 -r1.115 nsContentUtils.h
>--- content/base/public/nsContentUtils.h	4 Jan 2007 22:31:23 -0000	1.115
>+++ content/base/public/nsContentUtils.h	25 Jan 2007 16:35:58 -0000
>@@ -47,16 +47,17 @@
> #include "nsIStatefulFrame.h"
> #include "nsIPref.h"
> #include "nsINodeInfo.h"
> #include "nsNodeInfoManager.h"
> #include "nsContentList.h"
> #include "nsDOMClassInfoID.h"
> #include "nsIClassInfo.h"
> #include "nsIDOM3Node.h"
>+#include "nsDataHashtable.h"
> 
> class nsIDOMScriptObjectFactory;
> class nsIXPConnect;
> class nsINode;
> class nsIContent;
> class nsIDOMNode;
> class nsIDocument;
> class nsIDocShell;
>@@ -93,16 +94,31 @@ struct JSRuntime;
> class nsIXTFService;
> #endif
> #ifdef IBMBIDI
> class nsIBidiKeyboard;
> #endif
> 
> extern const char kLoadAsData[];
> 
>+enum EventNameType {
>+  EventNameType_HTML = 0x0001,
>+  EventNameType_XUL = 0x0002,
>+  EventNameType_SVGGraphic = 0x0004, // svg graphic elements
>+  EventNameType_SVGSVG = 0x0008, // the svg element
>+
>+  EventNameType_HTMLXUL = 0x0003,
>+  EventNameType_All = 0xFFFF,
>+};
>+
>+struct EventNameMapping {
>+  PRUint32  mId;
>+  PRInt32 mType;
>+};
>+
> class nsContentUtils
> {
> public:
>   static nsresult Init();
> 
>   // You MUST pass the old ownerDocument of aContent in as aOldDocument and the
>   // new one as aNewDocument.  aNewParent is allowed to be null; in that case
>   // aNewDocument will be assumed to be the parent.  Note that at this point
>@@ -760,16 +776,35 @@ public:
>   static nsresult DispatchTrustedEvent(nsIDocument* aDoc,
>                                        nsISupports* aTarget,
>                                        const nsAString& aEventName,
>                                        PRBool aCanBubble,
>                                        PRBool aCancelable,
>                                        PRBool *aDefaultAction = nsnull);
> 
>   /**
>+   * Determines if an event attribute name (such as onclick) is valid for
>+   * a given element type. Types are from the EventNameType enumeration
>+   * defined above.
>+   *
>+   * @param aName the event name to look up
>+   * @param aType the type of content
>+   */
>+  static PRBool IsEventAttributeName(nsIAtom* aName, PRInt32 aType);
>+
>+  /**
>+   * Return the event id for the event with the given name. The name is the
>+   * event name with the 'on' prefix. Returns NS_USER_DEFINED_EVENT if the
>+   * event doesn't match a known event name.
>+   *
>+   * @param aName the event name to look up
>+   */
>+  static PRUint32 GetEventId(nsIAtom* aName);
>+
>+  /**
>    * Used only during traversal of the XPCOM graph by the cycle
>    * collector: push a pointer to the listener manager onto the
>    * children deque, if it exists. Do nothing if there is no listener
>    * manager.
>    *
>    * Crucially: does not perform any refcounting operations.
>    *
>    * @param aNode The node to traverse.
>@@ -922,16 +957,19 @@ public:
>    * Utility method that checks if a given node has any non-empty
>    * children.
>    * NOTE! This method does not descend recursivly into elements.
>    * Though it would be easy to make it so if needed
>    */
>   static PRBool HasNonEmptyTextContent(nsINode* aNode);
> 
> private:
>+
>+  static void InitializeEventTable();
>+
>   static nsresult doReparentContentWrapper(nsIContent *aChild,
>                                            JSContext *cx,
>                                            JSObject *aOldGlobal,
>                                            JSObject *aNewGlobal,
>                                            nsIDocument *aOldDocument,
>                                            nsIDocument *aNewDocument);
> 
>   static nsresult EnsureStringBundle(PropertiesFile aFile);
>@@ -957,16 +995,18 @@ private:
>   static nsIPrefBranch *sPrefBranch;
> 
>   static nsIPref *sPref;
> 
>   static imgILoader* sImgLoader;
> 
>   static nsIConsoleService* sConsoleService;
> 
>+  static nsDataHashtable<nsISupportsHashKey, EventNameMapping>* sEventTable;
>+
>   static nsIStringBundleService* sStringBundleService;
>   static nsIStringBundle* sStringBundles[PropertiesFile_COUNT];
> 
>   static nsIContentPolicy* sContentPolicyService;
>   static PRBool sTriedToGetContentPolicy;
> 
>   static nsILineBreaker* sLineBreaker;
>   static nsIWordBreaker* sWordBreaker;
>Index: content/base/src/nsContentUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v
>retrieving revision 1.197
>diff -u -p -8 -r1.197 nsContentUtils.cpp
>--- content/base/src/nsContentUtils.cpp	4 Jan 2007 22:31:24 -0000	1.197
>+++ content/base/src/nsContentUtils.cpp	25 Jan 2007 16:35:58 -0000
>@@ -129,16 +129,18 @@ static NS_DEFINE_CID(kXTFServiceCID, NS_
> #include "nsEscape.h"
> #include "nsICharsetConverterManager.h"
> #include "nsIEventListenerManager.h"
> #include "nsAttrName.h"
> #include "nsIDOMUserDataHandler.h"
> #include "nsIFragmentContentSink.h"
> #include "nsContentCreatorFunctions.h"
> #include "nsTPtrArray.h"
>+#include "nsGUIEvent.h"
>+#include "nsMutationEvent.h"
> 
> #ifdef IBMBIDI
> #include "nsIBidiKeyboard.h"
> #endif
> #include "nsCycleCollectionParticipant.h"
> 
> // for ReportToConsole
> #include "nsIStringBundle.h"
>@@ -160,16 +162,17 @@ nsINameSpaceManager *nsContentUtils::sNa
> nsIIOService *nsContentUtils::sIOService;
> #ifdef MOZ_XTF
> nsIXTFService *nsContentUtils::sXTFService = nsnull;
> #endif
> nsIPrefBranch *nsContentUtils::sPrefBranch = nsnull;
> nsIPref *nsContentUtils::sPref = nsnull;
> imgILoader *nsContentUtils::sImgLoader;
> nsIConsoleService *nsContentUtils::sConsoleService;
>+nsDataHashtable<nsISupportsHashKey, EventNameMapping>* nsContentUtils::sEventTable = nsnull;
> nsIStringBundleService *nsContentUtils::sStringBundleService;
> nsIStringBundle *nsContentUtils::sStringBundles[PropertiesFile_COUNT];
> nsIContentPolicy *nsContentUtils::sContentPolicyService;
> PRBool nsContentUtils::sTriedToGetContentPolicy = PR_FALSE;
> nsILineBreaker *nsContentUtils::sLineBreaker;
> nsIWordBreaker *nsContentUtils::sWordBreaker;
> nsVoidArray *nsContentUtils::sPtrsToPtrsToRelease;
> nsIJSRuntimeService *nsContentUtils::sJSRuntimeService;
>@@ -305,16 +308,116 @@ nsContentUtils::Init()
>     }
>   }
> 
>   sInitialized = PR_TRUE;
> 
>   return NS_OK;
> }
> 
>+void
>+nsContentUtils::InitializeEventTable() {

Hook this function up to nsContentUtils::Init and make it return a boolean to indicate success/failure. That way you won't need to constantly check if the hash is initialized.


>+nsContentUtils::IsEventAttributeName(nsIAtom* aName, PRInt32 aType)
>+{
>+  if (!sEventTable)
>+    InitializeEventTable();

I.e. this can be removed, as well as the nullcheck below.

>+  EventNameMapping mapping;
>+  return (sEventTable &&
>+          sEventTable->Get(aName, &mapping) &&
>+          mapping.mType & aType);

I think it would be worth checking that the first two characters in the atom is 'o' and 'n' like nsGenericHTMLElement::IsEventName did, given that in most cases they're not.

sr=sicking with that fixed.

Comment 7

[Neil Deakin]

> Hook this function up to nsContentUtils::Init and make it return a
> boolean to indicate success/failure. That way you won't need to constantly
> check if the hash is initialized.

ContentUtils::Init is called before the atom tables are initialized so this doesn't work. I didn't want to change this initialization order in case of some other dependency. Perhaps there isn't one?

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 252786 [details] [diff] [review]
fix issues described above

Reviewing SVG changes:

+    { &nsGkAtoms::onload,                        { NS_LOAD, EventNameType_All }},
+    { &nsGkAtoms::onunload,                      { NS_PAGE_UNLOAD,
+                                                 (EventNameType_HTMLXUL | EventNameType_SVGSVG) }},
+    { &nsGkAtoms::onabort,                       { NS_IMAGE_ABORT, EventNameType_All }},
+    { &nsGkAtoms::onerror,                       { NS_LOAD_ERROR, EventNameType_All }},

The above four should not match for SVG. In SVG the 'onload' attribute adds a listener for the 'SVGLoad' event, not the 'load' event. Same idea for the others.

+   ,{ &nsGkAtoms::onSVGLoad,                     { NS_SVG_LOAD, EventNameType_SVGGraphic }},

Correct.

+    { &nsGkAtoms::onSVGUnload,                   { NS_SVG_UNLOAD, EventNameType_SVGGraphic }},
+    { &nsGkAtoms::onSVGAbort,                    { NS_SVG_ABORT, EventNameType_SVGGraphic }},
+    { &nsGkAtoms::onSVGError,                    { NS_SVG_ERROR, EventNameType_SVGGraphic }},
+    { &nsGkAtoms::onSVGResize,                   { NS_SVG_RESIZE, EventNameType_SVGGraphic }},
+    { &nsGkAtoms::onSVGScroll,                   { NS_SVG_SCROLL, EventNameType_SVGGraphic }},
+    { &nsGkAtoms::onSVGZoom,                     { NS_SVG_ZOOM, EventNameType_SVGGraphic }},

These six should all be EventNameType_SVGSVG _and_ EventNameType_SVGGraphic. All event attributes for graphic elements are also valid for the root <svg> tag in SVG 1.1.

+    { &nsGkAtoms::onzoom,                        { NS_SVG_ZOOM,
+                                                 (EventNameType_SVGGraphic | EventNameType_SVGSVG) }}

This line shouldn't exist.

Comment 9

[Jonathan Watt [:jwatt]]

Argh. I got that mixed up myself of course. onSVGLoad should be EventNameType_SVGSVG | EventNameType_SVGGraphic. The other six should be EventNameType_SVGSVG only.

Comment 10

[Neil Deakin]

Attachment: newer patch

I'm not sure I follow the last comments. The svg spec says that 'onload', 'onunload', 'onzoom' are all valid attributes.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 255793 [details] [diff] [review]
newer patch

I seem to have been a little confused in my previous comments, but this patch still seems wrong to me:

>+  EventNameType_SVGAttribute = 0x10000, // svg elements but only the attribute form

I guess this is a leftover from an experiment that should be removed?

>+#ifdef MOZ_SVG
>+   ,{ &nsGkAtoms::onSVGLoad,                     { NS_SVG_LOAD,
>+                                                 (EventNameType_SVGSVG | EventNameType_SVGGraphic) }},
>+    { &nsGkAtoms::onSVGUnload,                   { NS_SVG_UNLOAD, EventNameType_SVGSVG }},
>+    { &nsGkAtoms::onSVGAbort,                    { NS_SVG_ABORT, EventNameType_SVGSVG }},
>+    { &nsGkAtoms::onSVGError,                    { NS_SVG_ERROR, EventNameType_SVGSVG }},
>+    { &nsGkAtoms::onSVGResize,                   { NS_SVG_RESIZE, EventNameType_SVGSVG }},
>+    { &nsGkAtoms::onSVGScroll,                   { NS_SVG_SCROLL, EventNameType_SVGSVG }},
>+    { &nsGkAtoms::onSVGZoom,                     { NS_SVG_ZOOM, EventNameType_SVGSVG }},

These will allow onSVG attributes such as <svg onSVGLoad="..."> which we don't want to allow. It seems like EventNameMapping.mType should never match anything for these cases, no?

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 255793 [details] [diff] [review]
newer patch

>+    { &nsGkAtoms::onresize,                      { NS_RESIZE_EVENT, EventNameType_HTMLXUL }},

Should include EventNameType_SVGSVG.

Comment 13

[Neil Deakin]

> 
> These will allow onSVG attributes such as <svg onSVGLoad="..."> which we don't
> want to allow. It seems like EventNameMapping.mType should never match anything
> for these cases, no?

The onSVG* attributes are filtered out in nsSVGElement::IsElementEventName

Comment 14

[Jonathan Watt [:jwatt]]

Ah, true, I missed that change. Still, to me it would seem simpler and cleaner to add:

EventNameType_None = 0x0000

to the enum and use that for the group in comment 11.

Comment 15

[Neil Deakin]

Attachment: address latest comments

Comment 16

[Bill Gianopoulos [:WG9s]]

This seems to have caused bug 376428.