1466208 - Cleanup and fix bugs in PresShell::HandleEvent

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement, P2)

Description

[Mats Palmgren (:mats)]

Follow-up from bug 1430813 comment #21:
> PresShell::HandleEvent is a horrible mess.
> Here we might reassign 'frame' to the root frame of some shell ancestor:
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#6911
> but the rest of HandleEvent quite happily continues to use 'this'
> to access members etc.
> 
> The IME block at the end for example makes the assumption that
> 'frame' belongs to 'this':
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#7268
> It even assigns mCurrentEventFrame = frame which is clearly
> a recipe for disaster (exploitable UAFs).

Although the code lives under layout/ it seems this code is mostly doing
event handling / IME, so I'm assuming DOM Events is appropriate.

Comment 1

[Mats Palmgren (:mats)]

This bug is scary.  Can we find an owner for it please?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mats Palmgren (:mats) from comment #1)
> This bug is scary.  Can we find an owner for it please?

I'd like to work on this when I have much time. Do you think when should we fix this by?
(FYI: I'm working on redesigning editor for InputEvent.inputType and beforeinput event. Therefore, I don't have much time at least for a couple of months.)

Comment 3

[Andrew Overholt [:overholt]]

(For comment 2. I agree that Masayuki would be great for this, assuming timelines align.)

Comment 4

[Mats Palmgren (:mats)]

Sure, sounds good.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that it is really risky method to change. So, we should fix this bug slowly for detecting regressions.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e19e496ba8cff3694a109531df9273fae22dec

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e71982973369495dde0d280d790ab62b56a897b

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d045d68076e62557a54633b18ace3e2e79316111

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, I was thinking that the best solution is to split each block in HandleEvent() and HandleEventInternal() to methods of PresShell. However, this approach makes too many small methods into PresShell. And badly, some of them may be designed only for specific situations and need to take caller's local variables.

PresShell is a class not only for event handling. So, for grouping event handling helpers, I think that we should create a nexted stack based class called EventHandler and HandleEvent() should use it. Then, methods can share some variables with members.

Comment 10

[Olli Pettay [:smaug]]

I think I like that approach. It could lead to quite elegant code.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f9cc1ea88cec9ac9a1850a8746626f4a490370

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27c0ae1d079f9d92c50921f3ec44ab4d50951aff

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39af5b6e26e5b15de5aec39a5ac4c1563d5e14c7

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac0454f269aba3e1e2b4ca53182bb4bc8d564049

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b138c9351e71a294c10dd7411bf23247a26a9a

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf2a0f3b4f2351e78809472444bdca1e51fc270

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 1: Create stack class to handle events in PresShell

PresShell::HandleEvent() and PresShell::HandleEventInternal() are too big.
Additionally, we have a lot of methods used only by them. So, if we'll
split those big methods, PresShell will have a lot of small methods which
are not grouped as a part of event handling. That's too bad because some
of them may depend on the calling order, etc.

So, for grouping them, PresShell should create a stack class instance to handle
each event. Then, we can store shared information in it only while we're
handling an event.

This patch creates PresShell::EventHandler and PresShell methods become
wrappers of the stack class, but this patch does not change any logic in the
code, i.e., just reorganizing existing methods.

Note that HandleEventWithTarget() and HandleEventInternal() need to take
WidgetEvent rather than WidgetGUIEvent. Additionally, some other methods
require WidgetGUIEvent to refer WidgetGUIEvent::mWidget. Therefore, this
patch does not make the new class store the event as a member.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub

PresShell::EventHandler::HandleEvent() is too big. That makes us difficult to
understand the flow of them. So, first of all, we should split the method to
smaller chunks. Then, we can understand what we're doing in HandleEvent() more.

This patch creates MaybeHandleEventWithAccessibleCaret() for first handling
block in HandleEvent(). Note that the following patch will clean it up.
I.e., this patch just moves the existing block into the new method.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style

Because of spinning out from PresShell::EventHandler::HandleEvent(), we can use
early-return style in MaybeHandleEventWithAccessibleCaret(). This patch
rewrites MaybeHandleEventWithAccessibleCaret() with the style.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event

PresShell::HandleEvent() treats capturing content only when received event is
related to pointing device. And it's used in 2 purposes. One is for computing
to target document of coming event. The other is for handling events using
coordinates. Therefore, if we create a helper method to retrieve it, we can
move the variable into smaller blocks.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event

In some cases, PresShell::EventHandler::HandleEvent() needs to call
HandleEvent() of another instance.

For retrieving the instance, we need to compute retarget document first.
This patch makes new method to retrieve it. The following patch will clean
up it.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument()

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance

Next, we need to look for a frame for first parameter of calling
PresShell::HandleEvent() of another PresShell instance. This patch creates
PresShell::EventHandler::GetFrameForHandlingEventWith() to do it.

Unfortunately, the result is used in 3 patterns. One is, the caller should
stop handling the event. Another one is, the caller should keep handling
the event by itself. The other is, the caller should call
PresShell::HandleEvent() of different PresShell instance. Therefore, this
patch makes the method take aFrame of the caller. Then, the caller can check
the last 2 patterns with check the result is same as aFrame. This is not so
smart approach, but I have no better idea without adding a bool argument or
making the return type bool and adding out argument of nsIFrame.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith()

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary

Let's move the redirection of coming event in
PresShell::EventHandler::HandleEvent() into a method. This makes the caller
easier to read.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event

It may not be safe to handle events even when
PresShell::EventHandler::HandleEvent(). In such case, we need to discard
received events with notifying somebody. This patch move this rare case
jobs into the new method, MaybeDiscardEvent(). Then, the caller, HandleEvnet(),
becomes easier to read.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aeaa6c830bad2078df9469c3856a8e58d7e02b4

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8d00382e2da27d9861c0ce96cfd51597dd9482

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51b5293e59d136fbb9161bd5db8f73065f90b294

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66c3b5b15c8e
part 1: Create stack class to handle events in PresShell r=smaug

Comment 31

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/66c3b5b15c8e

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=681b23e26402ae5861928ff8b2cec3402c36c78f

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b7c7e0317ef
part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub r=smaug

Comment 34

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8b7c7e0317ef

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7c21360d5b51e194039626e1ff9e46a0951e8e

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c650507f7fee
part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style r=smaug

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4ce30ac236c687242d5707f4bbc9c21f1e43249

Comment 38

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c650507f7fee

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d38de19e569
part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event r=smaug

Comment 40

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9d38de19e569

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc12fba77e15a27960e4ef2bdb50ac75b82c8228

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7276db69abb703639acfaf648064652526203016

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e9c7b742cb9
part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event r=smaug

Comment 44

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7e9c7b742cb9

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc3dd03f8f4a2fbc8866cf6b688910afa093d46

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/646991effcda
part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument() r=smaug

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da405ee8d118dd5a1b605e96e4767613681dc97

Comment 48

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/646991effcda

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f2b49f3ed0327002885b1f8238035d04eba2824

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7930893f8f0
part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance r=smaug

Comment 51

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/e7930893f8f0

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea7d56b00394
part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith() r=smaug

Comment 53

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ea7d56b00394

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06d2f0093f58c1880c721b841cc8d74a8c2d2b1

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a1238e37ac2a071567747c7061286db11916371

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d868390c454
part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary r=smaug

Comment 57

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1d868390c454

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d612e0b0258
part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event r=smaug

Comment 59

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8d612e0b0258

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d7f57b2bc56fe7e9f25dfa993447c28147ed72

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()

PresShell::EventHandler::HandleEvent() discards or puts off to dispatch
the handling event if it's a keyboard event and event dispatching is
suppressed by the document.

This patch moves the block into the new method for making HandleEvent() simpler.

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent()

There is an unclear variable frame in PresShell::EventHandler::HandleEvent().
It's overwritten with different frame and its meanings is changed sometimes.
Finally, it's necessary only in the if (aGUIEvent->IsUsingCoordinates())
block. Therefore, we can move it into the block and rename it when them for
each purpose.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles()

PresShell::EventHandler::HandleEvent() tries to flush pending animation first
when it decides frame to handle events using coordinates. This patch moves
the code into the new method.

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent()

In some reasons, handling event should be handled in specific frame even if
the coordinates are out of the frame. PresShell::EventHandler::HandleEvent()
computes it with popups, capturing content, etc. This patch moves the blocks
into new method for making HandleEvent() simpler.

Note that most of the code is just moved. The following patch will clean it
up.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent()

PresShell::EventHandler::ComputeRootFrameToHandleEvent() computes root frame
to handle event with popup frame and/or capturing content. The former result
can be rewritten with the latter. So, for cleaning it up with early return
style, we need to split it to 2 methods.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91dfdbdede5fafb84097e2abb7494c7b6668d7bd

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7c6fdd0989ba264258224bd32b83fe3ea7741b4

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fd1b72f66c0
part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent() r=smaug

Comment 70

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7fd1b72f66c0

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8420442ba89
part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent() r=smaug

Comment 72

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d8420442ba89

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e170eeadd50fd6e22092c2e619c2046f329cce6d

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7276fd9b8354035ce4c43327834e20ce40dbc63

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da71b4d4ad40
part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles() r=smaug

Comment 76

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/da71b4d4ad40

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/546288d07c7c
part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug

Comment 78

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/546288d07c7c

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3282d059aa48323d476cf8ce7c572fe9683670

Comment 80

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edae943c21dee3ec682a068d87e5cae9e49b0711

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41be96f0dbbb
part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug

Comment 82

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/41be96f0dbbb

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b79d7da6617b
part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style r=smaug

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3472bb3a9c50507ce0f33646ec1d9eb6f65e871a

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b44755c3153259215a2be78daf3997564cf4a16

Comment 86

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b79d7da6617b

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdc27350d5389d998e1b9873288d353be2987abb

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame

This patch splits the part handling event when there is pointer capturing
content but it does not have frame.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent()

This patch moves the block, which discard or put the event into the delayed event
queue if handling event is a mouse event, to new method.

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 19: Group PresShell, nsIFrame and nsIContent with a struct

PresShell::EventHandler::HandleEvent() looks for PresShell, nsIFrame and
nsIContent a lot for aGUIEvent. Sometimes part of them are modified,
otherwise, all of them are modified by some reasons. Therefore, for
splitting each of the modifiers into separated methods, we need a struct
for making them as a group and usable for in/out parameter.

(If you have some ideas of better name, let me know.)

Comment 91

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument()

Now, we can create methods to update event target into EventTargetData().
This moves a block in PresShell::EventHandler::HandleEvent() to retarget
to active document into the new method.

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame()

This patch moves the block to compute event target of the event using
coordinates into the new method of PresShell::EventHandler::EventTargetData.

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8ad6016347f
part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame r=smaug

Comment 95

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/d8ad6016347f

Comment 96

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d70e9638ff75
part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent() r=smaug

Comment 97

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d70e9638ff75

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86b52991b24a
part 19: Group PresShell, nsIFrame and nsIContent with a struct r=smaug

Comment 99

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/86b52991b24a

Comment 100

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bcbf6ebcd45
part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() r=smaug

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cb13cbe5f05
part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style r=smaug

Comment 102

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1bcbf6ebcd45

Comment 103

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4cb13cbe5f05

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a53016221d4
part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame() r=smaug

Comment 105

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4a53016221d4

Comment 106

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 23: Create PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint()

We cannot move each block into separated methods while computing EventTargetData
because we need to check capturing contents, etc. Therefore, only each block
should be moved to separated methods for now.

This moves a block which computes event target from point of the event. If
this can be moved to EventTargetData, it might be easier to understand, but
its helper method GetFrameToHandleNonTouchEvent() requires to access members
of EventHandler. Therefore, we need to treat EventTargetData as an out param
of the new method.

Comment 107

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 24: Move `overrideClickTarget` into EventTargetData

Currently, PresShell::EventHandler::HandleEvent() sets overrideClickTarget
only when Pointer Events is enabled and there is pointer capturing content,
and this is computed while dispatching a pointer event.

So, if we move it into EventTargetData, we can move the pointer event
dispatching block into a separated method and caller can receive it with
an EventTargetData instance which is anyway necessary to receive new
target frame after dispatching a pointer event.

Comment 108

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 25: Create PresShell::EventHandler::DispatchPrecedingPointerEvent()

Now, we can move the block dispatching preceding pointer event to separated
method. Then, we can hide the complicated retarget process after dispatching
a pointer event from HandleEvent().

Comment 109

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 26: Create PresShell::EventHandler::EventTargetData::UpdateTouchEventTarget()

After dispatching pointer events, PresShell::EventHandler::HandleEvent()
updates event target only when the event is a touch event. We should do it in
a new method of EventTargetData.

Although I don't know why this is done in
PresShell::EventHandler::DispatchPrecedingPointerEvent().

Comment 110

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 27: Create PresShell::EventHandler::HandleEventUsingCoordinates()

Now, the block in HandleEvent(), which handles event using coordinates is
less than 200 lines. Perhaps, this is good amount to be split to a method.

This patch just moves the block to a new method.

Comment 111

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 28: Make PresShell::EventHandler::HandleEvent() handle non-using-coordinates events without frame before with frame case

When the event is not handled with coordinates and there is no frame for
mPresShell, PresShell::EventHandler::HandleEvent() handles the events
simpler than the case there is a frame. Therefore, this patch moves the
else block of if (aFrame) and reduce the indent of if (aFrame) case.

Comment 112

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 29: Create PresShell::EventHandler::ComputeFocusedEventTargetElement()

Most remaining code in PresShell::EventHandler::HandleEvent() is what computes
event target of the event which should be handled on focused content. This
patch moves the part to the new method.

Additionally, moves nsIPresShell::gKeyDownTarget to
EventHandler::sLastKeyDownEventTargetElement and make it use StaticRefPtr.

Finally, for using Element* instead of nsIContent*, changes the result type
of Document::GetUnfocusedKeyEventTarget() to Element*.

Comment 113

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload

If focused element is in another document,
PresShell::EventHandler::HandleEvent() needs to retarget the event to another
PresShell. This patch moves the case into new overload method,
MaybeHandleEventWithAnotherPresShell().

Additionally, removes PresShell::HandleRetargetedEvent() and makes
EventHandler::HandleRetargetedEvent() non-public because the new method
is the only user of them.

Comment 114

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell()

The remaining part of PresShell::EventHandler::HandleEvent() does:

1. Handles the event at focused content.
2. Handles the event with given frame which is a frame for mPresShell.

For making them clearer, this patch moves them into new methods.

Comment 115

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class

With splitting HandleEvent() a lot, it becomes more difficult to keep
managing each set of calling PushCurrentEventInfo() and
PopCurrentEventInfo(). So, EventHandler should have a helper class
to push and pop current event info into/from the stack.

Comment 116

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 33: Create PresShell::EventHandler::RecordEventHandlingResponsePerformance()

PresShell::EventHandler::HandleEventInsternal() recodes event handling
response performance with telemetry after it dispatches the event. We can move
it into new method simply.

Comment 117

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 34: Create a helper class, PresShell::EventHandler::HandlingTimeAccumulator()

PresShell::EventHandler::HandleEventInternal() needs to accumulate event
handling time per each event type. The handling start time needs to be
recoded before sending EventStateManager. Therefore, this patch makes the
helper class which is a stack class, recodes current time at construction
and calls Telemetry::AccumulateTimeDelta() at destruction automatically.

Comment 118

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 35: Reduce one indent level in PresShell::EventHandler::HandleEventInternal()

If aEvent requires frame but there is no event target,
PresShell::EventHandler::HandleEventInternal() just records the response
time. So, we can reduce one indent level in the big method.

Note that I'm not sure recoding the response time in such case because
the good values may make the average and median better. But this is
out of scope of bug 1466208.

Comment 119

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 36: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent()

If Shift state of eContextMenu event is active, we make it not fired on
web content. Additionally, if it's not time to open context menu, we shouldn't
dispatch it into the DOM. The new method prepare and check them.

Comment 120

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 37: Move trusted eMouseMove event preparation into the previous switch-case block

Oddly, there are two trusted eMouseMove preparation code in
PresShell::EventHandler::HandleEventInternal(). One is in the switch
statement which is used only when aEvent is trusted. The other is after
TouchManager::PreHandleEvent() is called and after
AutoHandlingUserInputStatePusher is created. However, both of them do
nothing if the event is eMouseMove. Therefore, we can move the latter
into the former.

Comment 121

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 38: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent()

The first switch statement of PresShell::EventHandler::HandleEventInternal()
has 2 jobs:

• Prepare something for specific event type.
• Record the preparation time of some types of events to telemetry.

This intermixed code is not easy to understand and somebody may add new
preparation after recording them. So, even though the preparation time
becomes worse a couple of milliseconds, we should split those jobs.

The patch moves the latter job into the new method.

Comment 122

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch()

PresShell::EventHandler::HandleEventInternal() may handle Escape key before
dispatching it in some cases. This requires too many lines for somebody who
investigate the method for the other events. Therefore, this patch moves it
into the new method.

Additionally, this patch creates WidgetKeyboardEvent::IsUserInteraction()
because PresShell::EventHandler refers same result in 2 places. Finally,
its condition is not enough to what the comment wants to do since it does
not check some modifier keys. Therefore, this patch makes it check all
possible modifier keys too.

Comment 123

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 40: Create PresShell::EventHandler::PrepareToDispatchEvent()

For making PresShell::EventHandler::HandleEventInternal() easier to read,
move the large switch statement for preparation into the new method.

Comment 124

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37f52c22967c
part 23: Create PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint() r=smaug

Comment 125

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Part 35 with ignoring whitespace changes

Comment 126

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/37f52c22967c

Comment 127

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51e72b19a617
part 24: Move `overrideClickTarget` into EventTargetData r=smaug

Comment 128

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/51e72b19a617

Comment 129

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9052ac46602
part 25: Create PresShell::EventHandler::DispatchPrecedingPointerEvent() r=smaug

Comment 130

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6fd8376d3ec
part 26: Create PresShell::EventHandler::EventTargetData::UpdateTouchEventTarget() r=smaug

Comment 131

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/c9052ac46602

Comment 132

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f05208c48b86
part 27: Create PresShell::EventHandler::HandleEventUsingCoordinates() r=smaug

Comment 133

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a6fd8376d3ec

Comment 134

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f05208c48b86

Comment 135

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deb832a494bb
part 28: Make PresShell::EventHandler::HandleEvent() handle non-using-coordinates events without frame before with frame case r=smaug

Comment 136

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/deb832a494bb

Comment 137

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/664b3bc4a449
part 29: Create PresShell::EventHandler::ComputeFocusedEventTargetElement() r=smaug

Comment 138

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/664b3bc4a449

Comment 139

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29a17314e008
part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class r=smaug

Comment 140

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/29a17314e008

Comment 141

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b5c05af88e2
part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload r=smaug

Comment 142

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a52bca8b0dd5
part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell() r=smaug

Comment 143

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5b5c05af88e2

Comment 144

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ad056528954
part 33: Create PresShell::EventHandler::RecordEventHandlingResponsePerformance() r=smaug

Comment 145

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a52bca8b0dd5

Comment 146

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 41: Create PresShell::EventHandler::DispatchEvent()

This is the part which actually handles the event. The new method should
notify EventStateManager of dispatching event before and after that, and
actually dispatch the event into the DOM.

Comment 147

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 42: Clean up PresShell::EventHandler::DispatchEvent() with using early-return style

Comment 148

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 43: Create PresShell::EventHandler::FinalizeHandlingEvent()

Finally, we should move the last switch statement in HandleEventInternal()
to the new method. Then, `HandleEventInternal() does nothing complicated
things by itself.

Comment 149

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 44: Rename PresShell::EventHandler::HandleEventInternal() to HandleEventWithCurrentEventInfo()

In my understanding, PresShell::EventHandler::HandleEvent() may redirect
the event to another class or PresShell first. Otherwise, it computes
event target and sets current event info of mPresShell to it. Then, calls
HandleEventInternal() to dispatch the event. Then, HandleEventInternal()
may handle the event before dispatch, and/or prepare to dispatch, then,
finally dispatches the event and finalize the state of mPresShell and the
event. Therefore, HandleEventInternal() actually handles the event, but
the word, "internal" is not explicitly explain its different points from
HandleEvent(). Therefore, I think that HandleEventWithCurrentEventInfo()
is better name since HandleEvent() considers the current event info.

Comment 150

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1466208 - part 45: Rename `aFrame` of `HandleEvent()` to `aFrameForPresShell`

Now, other methods taking aFrame of HandleEvent() names the argument as
aFrameForPresShell. So, HandleEvent()'s aFrame should also be renamed.

This patch renames it and adds MOZ_CAN_RUN_SCRIPT and comment to
nsIPresShell::HandleEvent().

This is the final patch for bug 1466208.

Comment 151

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5ad056528954

Comment 152

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f10044217e8f
part 34: Create a helper class, PresShell::EventHandler::HandlingTimeAccumulator() r=smaug

Comment 153

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/f10044217e8f

Comment 154

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22b988a39a32
part 35: Reduce one indent level in PresShell::EventHandler::HandleEventInternal() r=smaug

Comment 155

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/22b988a39a32

Comment 156

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/525dd00d5636
part 36: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug

Comment 157

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/525dd00d5636

Comment 158

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5e798d73ac6
part 37: Move trusted eMouseMove event preparation into the previous switch-case block r=smaug

Comment 159

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e5e798d73ac6

Comment 160

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e5e798d73ac6

Comment 161

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0c39e6d8d2c
part 38: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug

Comment 162

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/199af6c43953
part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch() r=smaug

Comment 163

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e0c39e6d8d2c
https://hg.mozilla.org/mozilla-central/rev/199af6c43953

Comment 164

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9482e1d252f
part 40: Create PresShell::EventHandler::PrepareToDispatchEvent() r=smaug

Comment 165

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f9482e1d252f

Comment 166

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9ed942f6b00
part 41: Create PresShell::EventHandler::DispatchEvent() r=smaug

Comment 167

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d9ed942f6b00

Comment 168

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a421f33761b
part 42: Clean up PresShell::EventHandler::DispatchEvent() with using early-return style r=smaug

Comment 169

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3a421f33761b

Comment 170

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8620e56a5f38
part 43: Create PresShell::EventHandler::FinalizeHandlingEvent() r=smaug

Comment 171

[Jorg K (CEST = GMT+2)]

Allow me this drive-by comment. As the TB sheriff I'm watching M-C merges and I see this bug every day :-)

Nice job, two parts missing to land, so you're done before the next branch date.

Comment 172

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8620e56a5f38

Comment 173

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+1) from comment #171)

Allow me this drive-by comment. As the TB sheriff I'm watching M-C merges and I see this bug every day :-)

Nice job, two parts missing to land, so you're done before the next branch date.

Thanks and yes, all patches will be landed 67 cycle. Note that the last patches have already been in the queue to land the autoland. So, this bug should be fixed in next 12 (or 24) hours.

Comment 174

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/88a92eb3865e

Comment 175

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/88a92eb3865e6482155111c684ba56bf73612ee3

Comment 176

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, failed to land part 45.

Comment 177

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4be0480f48d3
part 45: Rename `aFrame` of `HandleEvent()` to `aFrameForPresShell` r=smaug

Comment 178

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4be0480f48d3