Closed
Bug 238773
(nsdomevent_separate)
Opened 20 years ago
Closed 19 years ago
Separating nsDOMEvent into separate classes
Categories
(Core :: DOM: Events, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ilya.konstantinov+future, Assigned: ilya.konstantinov+future)
References
Details
Attachments
(5 files, 3 obsolete files)
128.67 KB,
patch
|
bryner
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
129.07 KB,
patch
|
Details | Diff | Splinter Review | |
128.91 KB,
patch
|
Details | Diff | Splinter Review | |
4.55 KB,
patch
|
bryner
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•20 years ago
|
Alias: nsdomevent_separate
Assignee | ||
Comment 1•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
Attachment #150591 -
Attachment is obsolete: true
Assignee | ||
Comment 3•20 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.
Comment 4•20 years ago
|
||
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•20 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•19 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•19 years ago
|
Attachment #154872 -
Flags: superreview?(jst)
Attachment #154872 -
Flags: review?(bryner)
Comment 7•19 years ago
|
||
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•19 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 9•19 years ago
|
||
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+
Comment 10•19 years ago
|
||
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•19 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•19 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.
Comment 14•19 years ago
|
||
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•19 years ago
|
Blocks: dom3_events
Assignee | ||
Comment 15•19 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: 19 years ago
Resolution: --- → FIXED
Comment 16•19 years ago
|
||
This regressed Tdhtml and probably Tp a bit.
Comment 17•19 years ago
|
||
Updated•19 years ago
|
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.
Comment 20•19 years ago
|
||
This caused bug 256464 (major IME regression).
Comment 21•19 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•19 years ago
|
||
Updated•19 years ago
|
Attachment #157795 -
Flags: review?(bryner)
Comment 23•19 years ago
|
||
Comment on attachment 157795 [details] [diff] [review] Regression fix sr=jst
Attachment #157795 -
Flags: superreview+
Assignee | ||
Comment 24•19 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 25•19 years ago
|
||
Comment on attachment 157795 [details] [diff] [review] Regression fix Make that r+sr=jst
Attachment #157795 -
Flags: review?(bryner) → review+
Comment 26•19 years ago
|
||
Regression fix checked in.
Comment 27•19 years ago
|
||
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•19 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.
![]() |
||
Comment 29•19 years ago
|
||
Ilya, post a patch (in a new bug?)?
You need to log in
before you can comment on or make changes to this bug.
Description
•