Closed Bug 238773 (nsdomevent_separate) Opened 16 years ago Closed 16 years ago

Separating nsDOMEvent into separate classes

Categories

(Core :: DOM: Events, enhancement)

x86
All
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ilya.konstantinov+future, Assigned: ilya.konstantinov+future)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

nsDOMEvent is a mishmash of code designed to handle all types of DOM events (by
implementing all nsIDOM*Event ifaces). I wish to separate nsDOMEvent into
separate classes in proper hierarchy. (This is preparing the grounds for my DOM
Level 3 events implementation.)

This is roughly what I want (for MouseEvent):

nsDOMMouseEvent implements nsIDOMMouseEvent, extends nsDOMUIEvent
nsDOMUIEvent implements nsIDOMUIEvent, extends nsDOMEvent
nsDOMEvent implements nsIDOMEvent

Compatibility note
------------------
Today, if you take a MouseEvent and get its' "keycode" property, it'll return
-1. With my change, it'll return null. Similar effects would happen when you
treating keyboard events as mouse events. I doubt anyone relies on that behavior.

I would be taking on this code-wise. This bug is for tracking.
Alias: nsdomevent_separate
Attached patch Initial patch (obsolete) — Splinter Review
A bit unfocused, this patch:
1. Separates events into classes.
2. Adds DOM3 ifaces for Event, UIEvent, KeyboardEvent, MouseEvent, TextEvent
and MutationEvent.
3. Adds the XML Events namespace ID, as required by DOM3 Events.
4. Fixes bug 246447 (can be stripped out).
5. Renames bryner's NS_DOMUI_ACTIVATE to NS_UI_ACTIVATE (and the same goes for
all other occurances of DOMUI), to prevent it from clashing with content/events
and to make a clean separation between the widget/ world (non-DOM-aware) and
the content/ world.
6. Additional VKs added for left/right modifier keys. DOM3 requires us to make
the distinction between them. Compatibility is preserved in KeyEvent by always
serving the old VKs.

Known issues:
1. TextEvent is provided only when input is thru IME. This is not what the spec
dictates.
2. When HTML events originate from the UI, UIEvent should be used. Currently,
Event is used. UIEvent should be always used for Scroll and Resize events.
3. Only gtk widget backend waschanged to make use of the left/right modifier
VKs.
Assignee: events → mozilla-bugzilla
Status: NEW → ASSIGNED
Attached patch Updated patch (obsolete) — Splinter Review
Changes:
1. Merges cleanly against trunk.
2. nsDOMUIEvent::GetView now AddRefs, like it should. (This bug caused
drag-and-drop to crash and burn.)

BTW,
1. nsDOMTextEvent::GetInputRange doesn't addref on the object it returns, but
its only user (the editor) addrefs on it after retrieving it. Ugly, but
apparently ... by design?
2. Is there some horrible bug in the trunk right now that causes it to open all
pages in the first window/tab? I sure hope it's not my code...
Attachment #150591 - Attachment is obsolete: true
- Fixed a real horrible bug, resulting from treating nsString.Equals... as a
strcmp-style comparison function.
- Adapted to merge nsEventListenerManager.cpp changes cleanly into trunk.
Blocks: 166240
Ilya: when you are ready, you should set the 'review ?' and 'superreview ?'
request-flags on your patches (in Edit), so that the other Mozilla developers
can review your code and provide feedback. If you think the patches are ready,
that is :)

I guess you should ask peterv, jst, caillon, sicking or bzbarsky for help.
The current patch also adds DOM 3 support, which should be a separate bug 
that'll take a separate review. Currently I've finished the work on separating 
the DOM3 stuff out of this patch and I will submit it for review soon. 
This patch is stripped from the DOM3 work I did. The DOM3 ifaces implementation
will come in a separate bug, after this patch goes in.
Attachment #150622 - Attachment is obsolete: true
Attachment #151105 - Attachment is obsolete: true
Attachment #154872 - Flags: superreview?(jst)
Attachment #154872 - Flags: review?(bryner)
Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

>--- content/base/src/nsGenericElement.cpp	30 Jul 2004 13:00:35 -0000	3.343
>+++ content/base/src/nsGenericElement.cpp	31 Jul 2004 18:08:34 -0000
>@@ -1156,8 +1156,9 @@ nsGenericElement::InternalIsSupported(co
>              PL_strcasecmp(f, "CSS") == 0 ||
>              PL_strcasecmp(f, "CSS2") == 0 ||
>              PL_strcasecmp(f, "Events") == 0 ||
>-//           PL_strcasecmp(f, "UIEvents") == 0 ||
>+             PL_strcasecmp(f, "UIEvents") == 0 ||
>              PL_strcasecmp(f, "MouseEvents") == 0 ||

I'm not sure we should claim to support UIEvents when we don't do DOMFocusIn
and DOMFocusOut...

Other than that, r=me.
Attachment #154872 - Flags: review?(bryner) → review+
DOMFocusIn and DOMFocusOut? Sure we do! Didn't you contribute this in the first
place (nsEvent message type NS_DOMUI_FOCUS... which I've renamed to NS_UI_FOCUS...)?
Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

What bryner said, and I'd prefer to see you add your email address in the
contributor section in the comments, like others tend to do.

sr=jst
Attachment #154872 - Flags: superreview?(jst) → superreview+
Duh, sorry, missed your last comment. Yeah, seems like we do support all that's
needed for claiming support for UIEvents now. Sweet! :-)
No changes since the previous patch, except for adding my email address to the
contributors list (as per jst's comment).
Same patch, without the s/nsIPresContext/nsPresContext/ bitrot. This patch
should apply cleanly on the trunk.
I find it hard to believe that recyling event objects matters at all for
performance. I propose just throwing that out, unless someone can demonstrate a
plausible need for it.
See comment 12 in bug 64696. Though I suppose we could try to take it out and
see if things have changed since then.
Blocks: dom3_events
Checked into trunk. We'll see about aviary once it bakes in the trunk for a 
while. Meanwhile I continue to the DOM3 bug (bug 256333). 
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This regressed Tdhtml and probably Tp a bit.
Attachment #156644 - Flags: review+
(In reply to comment #14)
> See comment 12 in bug 64696. Though I suppose we could try to take it out and
> see if things have changed since then.

Interesting. I still find it hard to believe.
I strongly recommend against doing this for Aviary or 1.7.
This caused bug 256464 (major IME regression).
There is one more regression - document.createEvent("KeyEvents") creates mouse
events now. Patch for nsEventListenerManager.cpp should be:

     if (str.EqualsIgnoreCase("KeyboardEvent") ||
         str.EqualsIgnoreCase("KeyEvents"))
-       return NS_NewDOMMouseEvent(aDOMEvent, aPresContext,
NS_STATIC_CAST(nsKeyEvent*,aEvent));
+       return NS_NewDOMKeyboardEvent(aDOMEvent, aPresContext,
NS_STATIC_CAST(nsKeyEvent*,aEvent));
    if (str.EqualsIgnoreCase("MutationEvent") ||
        str.EqualsIgnoreCase("MutationEvents"))
Attached patch Regression fixSplinter Review
Attachment #157795 - Flags: review?(bryner)
Comment on attachment 157795 [details] [diff] [review]
Regression fix

sr=jst
Attachment #157795 - Flags: superreview+
bryner's last patch looks good. (Hmmm...The compiler didn't warn me cause 
NS_NewDOMMouseEvent accepts an nsInputEvent, which nsKeyEvent inherits from.) 
Comment on attachment 157795 [details] [diff] [review]
Regression fix

Make that r+sr=jst
Attachment #157795 - Flags: review?(bryner) → review+
Regression fix checked in.
Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

>+      case NS_POPUP_EVENT:
>+        return NS_NewDOMPopupBlockedEvent(aDOMEvent, aPresContext, NS_STATIC_CAST(nsPopupBlockedEvent*,aEvent));

Shouldn't this have been NS_POPUPBLOCKED_EVENT?
From a short LXR search, it looks like all popup blocked events are now
generated with mEvent->eventStructType == NS_POPUPBLOCKED_EVENT, so both
content/events/src/nsDOMUIEvent.cpp and
content/events/src/nsEventListenerManager.cpp should be changed not to refer to
NS_POPUP_EVENT anymore.
Ilya, post a patch (in a new bug?)?
Depends on: 291725
You need to log in before you can comment on or make changes to this bug.