Cleanup and fix bugs in PresShell::HandleEvent
Categories
(Core :: DOM: UI Events & Focus Handling, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: masayuki)
References
Details
Attachments
(46 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1466208 - part 37: Move trusted eMouseMove event preparation into the previous switch-case block
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
6.27 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Updated•5 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
This bug is scary. Can we find an owner for it please?
Assignee | ||
Comment 2•5 years ago
|
||
(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•5 years ago
|
||
(For comment 2. I agree that Masayuki would be great for this, assuming timelines align.)
Assignee | ||
Comment 5•4 years ago
|
||
I think that it is really risky method to change. So, we should fix this bug slowly for detecting regressions.
Assignee | ||
Comment 6•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e19e496ba8cff3694a109531df9273fae22dec
Assignee | ||
Comment 7•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e71982973369495dde0d280d790ab62b56a897b
Assignee | ||
Comment 8•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d045d68076e62557a54633b18ace3e2e79316111
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
I think I like that approach. It could lead to quite elegant code.
Assignee | ||
Comment 11•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f9cc1ea88cec9ac9a1850a8746626f4a490370
Assignee | ||
Comment 12•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27c0ae1d079f9d92c50921f3ec44ab4d50951aff
Assignee | ||
Comment 13•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39af5b6e26e5b15de5aec39a5ac4c1563d5e14c7
Assignee | ||
Comment 14•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac0454f269aba3e1e2b4ca53182bb4bc8d564049
Assignee | ||
Comment 15•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b138c9351e71a294c10dd7411bf23247a26a9a
Assignee | ||
Comment 16•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf2a0f3b4f2351e78809472444bdca1e51fc270
Assignee | ||
Comment 17•4 years ago
|
||
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.
Assignee | ||
Comment 18•4 years ago
|
||
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.
Assignee | ||
Comment 19•4 years ago
|
||
Because of spinning out from PresShell::EventHandler::HandleEvent(), we can use
early-return style in MaybeHandleEventWithAccessibleCaret(). This patch
rewrites MaybeHandleEventWithAccessibleCaret() with the style.
Assignee | ||
Comment 20•4 years ago
|
||
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.
Assignee | ||
Comment 21•4 years ago
|
||
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.
Assignee | ||
Comment 22•4 years ago
|
||
Assignee | ||
Comment 23•4 years ago
|
||
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.
Assignee | ||
Comment 24•4 years ago
|
||
Assignee | ||
Comment 25•4 years ago
|
||
Let's move the redirection of coming event in
PresShell::EventHandler::HandleEvent() into a method. This makes the caller
easier to read.
Assignee | ||
Comment 26•4 years ago
|
||
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.
Assignee | ||
Comment 27•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aeaa6c830bad2078df9469c3856a8e58d7e02b4
Assignee | ||
Comment 28•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8d00382e2da27d9861c0ce96cfd51597dd9482
Assignee | ||
Comment 29•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51b5293e59d136fbb9161bd5db8f73065f90b294
Comment 30•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/66c3b5b15c8e part 1: Create stack class to handle events in PresShell r=smaug
Comment 31•4 years ago
|
||
bugherder |
Assignee | ||
Comment 32•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=681b23e26402ae5861928ff8b2cec3402c36c78f
Comment 33•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/8b7c7e0317ef part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub r=smaug
Comment 34•4 years ago
|
||
bugherder |
Assignee | ||
Comment 35•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7c21360d5b51e194039626e1ff9e46a0951e8e
Comment 36•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/c650507f7fee part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style r=smaug
Assignee | ||
Comment 37•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4ce30ac236c687242d5707f4bbc9c21f1e43249
Comment 38•4 years ago
|
||
bugherder |
Comment 39•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Assignee | ||
Comment 41•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc12fba77e15a27960e4ef2bdb50ac75b82c8228
Assignee | ||
Comment 42•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7276db69abb703639acfaf648064652526203016
Comment 43•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/7e9c7b742cb9 part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event r=smaug
Comment 44•4 years ago
|
||
bugherder |
Assignee | ||
Comment 45•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc3dd03f8f4a2fbc8866cf6b688910afa093d46
Comment 46•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/646991effcda part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument() r=smaug
Assignee | ||
Comment 47•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da405ee8d118dd5a1b605e96e4767613681dc97
Comment 48•4 years ago
|
||
bugherder |
Assignee | ||
Comment 49•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f2b49f3ed0327002885b1f8238035d04eba2824
Comment 50•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Comment 52•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/ea7d56b00394 part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith() r=smaug
Comment 53•4 years ago
|
||
bugherder |
Assignee | ||
Comment 54•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06d2f0093f58c1880c721b841cc8d74a8c2d2b1
Assignee | ||
Comment 55•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a1238e37ac2a071567747c7061286db11916371
Comment 56•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Comment 58•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Assignee | ||
Comment 60•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d7f57b2bc56fe7e9f25dfa993447c28147ed72
Assignee | ||
Comment 61•4 years ago
|
||
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.
Assignee | ||
Comment 62•4 years ago
|
||
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.
Assignee | ||
Comment 63•4 years ago
|
||
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.
Assignee | ||
Comment 64•4 years ago
|
||
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.
Assignee | ||
Comment 65•4 years ago
|
||
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.
Assignee | ||
Comment 66•4 years ago
|
||
Assignee | ||
Comment 67•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91dfdbdede5fafb84097e2abb7494c7b6668d7bd
Updated•4 years ago
|
Assignee | ||
Comment 68•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7c6fdd0989ba264258224bd32b83fe3ea7741b4
Comment 69•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/7fd1b72f66c0 part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent() r=smaug
Comment 70•4 years ago
|
||
bugherder |
Comment 71•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/d8420442ba89 part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent() r=smaug
Comment 72•4 years ago
|
||
bugherder |
Assignee | ||
Comment 73•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e170eeadd50fd6e22092c2e619c2046f329cce6d
Assignee | ||
Comment 74•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7276fd9b8354035ce4c43327834e20ce40dbc63
Comment 75•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/da71b4d4ad40 part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles() r=smaug
Comment 76•4 years ago
|
||
bugherder |
Comment 77•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/546288d07c7c part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug
Comment 78•4 years ago
|
||
bugherder |
Assignee | ||
Comment 79•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3282d059aa48323d476cf8ce7c572fe9683670
Assignee | ||
Comment 80•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edae943c21dee3ec682a068d87e5cae9e49b0711
Comment 81•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/41be96f0dbbb part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug
Comment 82•4 years ago
|
||
bugherder |
Comment 83•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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
Assignee | ||
Comment 84•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3472bb3a9c50507ce0f33646ec1d9eb6f65e871a
Assignee | ||
Comment 85•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b44755c3153259215a2be78daf3997564cf4a16
Comment 86•4 years ago
|
||
bugherder |
Assignee | ||
Comment 87•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdc27350d5389d998e1b9873288d353be2987abb
Assignee | ||
Comment 88•4 years ago
|
||
This patch splits the part handling event when there is pointer capturing
content but it does not have frame.
Assignee | ||
Comment 89•4 years ago
|
||
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.
Assignee | ||
Comment 90•4 years ago
|
||
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.)
Assignee | ||
Comment 91•4 years ago
|
||
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.
Assignee | ||
Comment 92•4 years ago
|
||
Assignee | ||
Comment 93•4 years ago
|
||
This patch moves the block to compute event target of the event using
coordinates into the new method of PresShell::EventHandler::EventTargetData.
Comment 94•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Comment 96•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/d70e9638ff75 part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent() r=smaug
Comment 97•4 years ago
|
||
bugherder |
Comment 98•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/86b52991b24a part 19: Group PresShell, nsIFrame and nsIContent with a struct r=smaug
![]() |
||
Comment 99•4 years ago
|
||
bugherder |
Comment 100•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/1bcbf6ebcd45 part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() r=smaug
Comment 101•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/4cb13cbe5f05 part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style r=smaug
Comment 102•4 years ago
|
||
bugherder |
Comment 103•4 years ago
|
||
bugherder |
Comment 104•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/4a53016221d4 part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame() r=smaug
Comment 105•4 years ago
|
||
bugherder |
Assignee | ||
Comment 106•4 years ago
|
||
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.
Assignee | ||
Comment 107•4 years ago
|
||
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.
Assignee | ||
Comment 108•4 years ago
|
||
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().
Assignee | ||
Comment 109•4 years ago
|
||
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()
.
Assignee | ||
Comment 110•4 years ago
|
||
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.
Assignee | ||
Comment 111•4 years ago
|
||
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.
Assignee | ||
Comment 112•4 years ago
|
||
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*
.
Assignee | ||
Comment 113•4 years ago
|
||
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.
Assignee | ||
Comment 114•4 years ago
|
||
The remaining part of PresShell::EventHandler::HandleEvent()
does:
- Handles the event at focused content.
- Handles the event with given frame which is a frame for
mPresShell
.
For making them clearer, this patch moves them into new methods.
Assignee | ||
Comment 115•4 years ago
|
||
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.
Assignee | ||
Comment 116•4 years ago
|
||
PresShell::EventHandler::HandleEventInsternal()
recodes event handling
response performance with telemetry after it dispatches the event. We can move
it into new method simply.
Assignee | ||
Comment 117•4 years ago
|
||
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.
Assignee | ||
Comment 118•4 years ago
|
||
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.
Assignee | ||
Comment 119•4 years ago
|
||
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.
Assignee | ||
Comment 120•4 years ago
|
||
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.
Assignee | ||
Comment 121•4 years ago
|
||
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.
Assignee | ||
Comment 122•4 years ago
|
||
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.
Assignee | ||
Comment 123•4 years ago
|
||
For making PresShell::EventHandler::HandleEventInternal()
easier to read,
move the large switch statement for preparation into the new method.
Comment 124•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/37f52c22967c part 23: Create PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint() r=smaug
Assignee | ||
Comment 125•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 126•4 years ago
|
||
bugherder |
Comment 127•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/51e72b19a617 part 24: Move `overrideClickTarget` into EventTargetData r=smaug
Comment 128•4 years ago
|
||
bugherder |
Comment 129•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/c9052ac46602 part 25: Create PresShell::EventHandler::DispatchPrecedingPointerEvent() r=smaug
Comment 130•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/a6fd8376d3ec part 26: Create PresShell::EventHandler::EventTargetData::UpdateTouchEventTarget() r=smaug
Comment 131•4 years ago
|
||
bugherder |
Comment 132•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/f05208c48b86 part 27: Create PresShell::EventHandler::HandleEventUsingCoordinates() r=smaug
Comment 133•4 years ago
|
||
bugherder |
Comment 134•4 years ago
|
||
bugherder |
Comment 135•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
Comment 137•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/664b3bc4a449 part 29: Create PresShell::EventHandler::ComputeFocusedEventTargetElement() r=smaug
Comment 138•4 years ago
|
||
bugherder |
Comment 139•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/29a17314e008 part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class r=smaug
Comment 140•4 years ago
|
||
bugherder |
Comment 141•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/5b5c05af88e2 part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload r=smaug
Comment 142•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/a52bca8b0dd5 part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell() r=smaug
Comment 143•4 years ago
|
||
bugherder |
Comment 144•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/5ad056528954 part 33: Create PresShell::EventHandler::RecordEventHandlingResponsePerformance() r=smaug
Comment 145•4 years ago
|
||
bugherder |
Assignee | ||
Comment 146•4 years ago
|
||
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.
Assignee | ||
Comment 147•4 years ago
|
||
Assignee | ||
Comment 148•4 years ago
|
||
Finally, we should move the last switch statement in HandleEventInternal()
to the new method. Then, `HandleEventInternal() does nothing complicated
things by itself.
Assignee | ||
Comment 149•4 years ago
|
||
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.
Assignee | ||
Comment 150•4 years ago
|
||
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•4 years ago
|
||
bugherder |
Comment 152•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/f10044217e8f part 34: Create a helper class, PresShell::EventHandler::HandlingTimeAccumulator() r=smaug
Comment 153•4 years ago
|
||
bugherder |
Comment 154•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/22b988a39a32 part 35: Reduce one indent level in PresShell::EventHandler::HandleEventInternal() r=smaug
Comment 155•4 years ago
|
||
bugherder |
Comment 156•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/525dd00d5636 part 36: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug
Comment 157•4 years ago
|
||
bugherder |
Comment 158•4 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•4 years ago
|
||
bugherder |
![]() |
||
Comment 160•4 years ago
|
||
bugherder |
Comment 161•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/e0c39e6d8d2c part 38: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug
Comment 162•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/199af6c43953 part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch() r=smaug
Comment 163•4 years ago
|
||
bugherder |
Comment 164•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/f9482e1d252f part 40: Create PresShell::EventHandler::PrepareToDispatchEvent() r=smaug
Comment 165•4 years ago
|
||
bugherder |
Comment 166•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/d9ed942f6b00 part 41: Create PresShell::EventHandler::DispatchEvent() r=smaug
Comment 167•4 years ago
|
||
bugherder |
Comment 168•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/3a421f33761b part 42: Clean up PresShell::EventHandler::DispatchEvent() with using early-return style r=smaug
Comment 169•4 years ago
|
||
bugherder |
Comment 170•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/8620e56a5f38 part 43: Create PresShell::EventHandler::FinalizeHandlingEvent() r=smaug
Comment 171•4 years ago
|
||
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•4 years ago
|
||
bugherder |
Assignee | ||
Comment 173•4 years ago
|
||
(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.
Assignee | ||
Updated•4 years ago
|
Comment 174•4 years ago
|
||
bugherder |
![]() |
||
Comment 175•4 years ago
|
||
Assignee | ||
Comment 176•4 years ago
|
||
Oh, failed to land part 45.
Updated•4 years ago
|
Comment 177•4 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/4be0480f48d3 part 45: Rename `aFrame` of `HandleEvent()` to `aFrameForPresShell` r=smaug
Updated•4 years ago
|
Comment 178•4 years ago
|
||
bugherder |
Description
•