124990 - Event ordering rewrite tracking bug

Status: RESOLVED FIXED

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

Description

[joki (gone)]

Event handling at the XP level is being rewritten to use two separate passes
though the event handlers, one for external script handlers on web pages and a
second for internal event handlers used to trigger browser actions.

This bug fix will affect a number of current bugs, opening this tracking bug to
better organize them.

Comment 1

[joki (gone)]

124990 and its associated bugs is not quite ready for checkin for 0.9.9. 
Maintaining high priority and moving to 1.0 for completion and further testing.

Comment 2

[joki (gone)]

Attachment: Patch from event branch

Comment 3

[joki (gone)]

Attachment: Updated patch

Comment 4

[joki (gone)]

Attachment: And now the patch in the right format...

Comment 5

[Marek Z. Jeziorek]

Marking it topembed+. However any event rewrite sounds scary to me and prone to
regressions. What do we get out of rewriting it (i.e., benefits of it)?

Comment 6

[joki (gone)]

What we gain is the ability to fix all the bugs blocked by this, a number of 
which are fairly important.  I agree, though, that it is risky.  With that in 
mind, saari and I have talked and decided to take a cautious route for getting 
this code in.  QA worked with it last week and it looks pretty good so far.  
We're planning to land it after moz1.0 and hopefully backport it to any release 
which needs the fixes it provides.  In addition, test builds of this branch 
will be available shortly for additional testing by anyone who wants to try it.

Comment 7

[Jaime Rodriguez, Jr.]

This patch appears to fix many event problems, bringing lots of goodness. Adding
nsbeta1, and ADT2 so that its on the ADT radar for MachV. 

Comment 8

[joki (gone)]

Attachment: Updated patch (no changes to patch, just adjusting for recent tree changes)

Comment 9

[joki (gone)]

Attachment: One more patch to fix update conflicts

Comment 10

[joki (gone)]

Attachment: zip with new files

New files from branch.	Can also be found on EVENTSREWRITE_20000211_BRANCH.

Comment 11

[joki (gone)]

Attachment: One last update

Comment 12

[saari (gone)]

Comment on attachment 79691 [details] [diff] [review]
One last update

heafty heafty heafy. r=saari

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 79691 [details] [diff] [review]
One last update

sr=jst

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like this was backed out because it caused regressions on Linux,
although there are no comments on the bug indicating that nor any indication in
the backout checkin comment of what the regressions were.  I heard some mention
on Friday of an inability to use Backspace in the address field of the compose
Window, although I can't find a corresponding bug.  However, there's another
problem I'm seeing (on Linux) which I suspect is related:  when I try to operate
the menus with the keyboard, I can open the menu with Alt-<shortcut>, but once I
do that no key events are accepted and I have to use the mouse.  (I can't use
Esc to close the menu or one of the shortcuts to select one of the items.)

Comment 15

[Brian Ryner (not reading)]

dbaron: yes, the problem with backspace in mail compose is the only reason I'm
aware of that this was backed out (well, that and joki going on vacation for a
week).

Comment 16

[saari (gone)]

dbaron, thanks for the comment, I'm sure Tom will look at that too once he gets
back. Sounds like there is one issue that is unique to linux that is causing
this stuff, Tom just couldn't find it before vacation so we backed it out for now.

Comment 17

[Dave Barrowman]

topembed-, embed per EDT triage.

Comment 18

[Jaime Rodriguez, Jr.]

nsbeta1- per the ADT. let's look at it for a future release.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK. So saari says the linux issues have been resolved.... could we possibly land
this at the start of the 1.2alpha cycle?  

Comment 20

[joki (gone)]

Attachment: Small addition to patch to prevent reopening of 138548

Here's the small bit that had to be added to the patch.  It fixes a bug in the
gtk widget lib which resulted in reused nsEvent structs causing data from
keydown events to get into keypress events.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 97710 [details] [diff] [review]
Small addition to patch to prevent reopening of 138548

sr=jst

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97710 [details] [diff] [review]
Small addition to patch to prevent reopening of 138548

r=bzbarsky, but our default linux build is "gtk", not "gtk2", no?

Looking at nsGtkEventHandler.cpp, it looks like
handle_key_press_event_for_text() and handle_key_press_event() may need a
similar change (both reuse nsKeyEvent structs).

Comment 23

[Brian Ryner (not reading)]

Yeah, what bz said.  The gtk2 build is experimental at this point, you need to
patch widget/src/gtk as well.

Comment 24

[Brian Ryner (not reading)]

Attachment: gtk1 patch to fix key event reusage

Comment 25

[Christopher Blizzard (:blizzard)]

Comment on attachment 101479 [details] [diff] [review]
gtk1 patch to fix key event reusage

sr=blizzard

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 101479 [details] [diff] [review]
gtk1 patch to fix key event reusage

r=dbaron

Comment 27

[Brian Ryner (not reading)]

Both the gtk and gtk2 patches have been checked in on the trunk.

Comment 28

[Brian Ryner (not reading)]

Attachment: main patch, updated to the trunk

Comment 29

[Brian Ryner (not reading)]

taking.

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

This checkin caused regression bug 179172: Context menus no longer work in
Mailnews thread or folder panes.

Comment 31

[Brian Ryner (not reading)]

This is checked in.  I'm tracking regressions on bug 180455.

Comment 32

[Ilya Konstantinov]

This patch implements an older version of the DOM Level 3 Events module; one
which had AddGroupedEventListener instead of AddEventListenerNS. I'll be
changing nsIDOM3EventTarget to remove those methods and add the up-to-date spec
methods instead, as part of my work on bug 256333. Since this iface is not
frozen, I guess it's okay.

Just FYI.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw. this patch added the non-frozen interfaces:
nsIDOM3DocumentEvent.idl
nsIDOM3EventTarget.idl
nsIDOMCustomEvent.idl
nsIDOMEventGroup.idl

to the sdk (the DOM3 Events spec has since changed at least some of these ifaces)

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

filed Bug 275220 about that last comment