Bug 238773 (nsdomevent_separate)

Separating nsDOMEvent into separate classes

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
16 years ago
5 years ago

People

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

Tracking

(Blocks 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Assignee

Description

16 years ago
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.
Assignee

Updated

16 years ago
Alias: nsdomevent_separate
Assignee

Comment 1

15 years ago
Posted 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
Assignee

Comment 2

15 years ago
Posted 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...
Assignee

Updated

15 years ago
Attachment #150591 - Attachment is obsolete: true
Assignee

Comment 3

15 years ago
- 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.
Assignee

Updated

15 years ago
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.
Assignee

Comment 5

15 years ago
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. 
Assignee

Comment 6

15 years ago
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
Assignee

Updated

15 years ago
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+
Assignee

Comment 8

15 years ago
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! :-)
Assignee

Comment 11

15 years ago
No changes since the previous patch, except for adding my email address to the
contributors list (as per jst's comment).
Assignee

Comment 12

15 years ago
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.
Assignee

Updated

15 years ago
Blocks: dom3_events
Assignee

Comment 15

15 years ago
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: 15 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).

Comment 21

15 years ago
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"))

Comment 22

15 years ago

Updated

15 years ago
Attachment #157795 - Flags: review?(bryner)
Comment on attachment 157795 [details] [diff] [review]
Regression fix

sr=jst
Attachment #157795 - Flags: superreview+
Assignee

Comment 24

15 years ago
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?
Assignee

Comment 28

14 years ago
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?)?
Assignee

Updated

14 years ago
Depends on: 291725
You need to log in before you can comment on or make changes to this bug.