When I'm hacking some XP level modules, I see a lot of methods take nsIDOM*Event and use its virtual methods. But if I tried to rewrite it with Widget*Event, calls of PreventDefault(), StopPropagation() and StopImmediatePropagation() block me to do it. So, I believe that if we move some methods of Event.cpp is implemented in WidgetEvent, we can make such methods simpler and use mMessage for comparing event types rather than string comparison. Smaug, do you have some comments about this idea?

Should be moved: StopPropagation() StopImmediatePropagation() StopCrossProcessForwarding() PreventDefault() Optional: DefaultPrevented() DefaultPreventedByContent() DefaultPreventedByOnlyChrome() IsTrusted()

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #1) > Should be moved: > > StopPropagation() > StopImmediatePropagation() > StopCrossProcessForwarding() > PreventDefault() > These look fine > DefaultPrevented() > DefaultPreventedByContent() > DefaultPreventedByOnlyChrome() > IsTrusted() I guess these too, though I'm not sure whether any widget level code should care about DefaultPreventedByContent() or DefaultPreventedByOnlyChrome()

Review commit: https://reviewboard.mozilla.org/r/41235/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41235/

Review commit: https://reviewboard.mozilla.org/r/41237/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41237/

Review commit: https://reviewboard.mozilla.org/r/41239/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41239/

mDefaultPreventedByChrome is hacky. When PresShell handles Escape key events in fullscreen mode, it prevents default of every Escape key events and dispatch it only into chrome. After that, it check mDefaultPreventedByChrome if at least one call of preventDefault() occurred in chrome. Therefore, if we shouldn't set both mDefaultPreventedByChrome and mDefaultPreventedByContent to true before dispatching an event. This the reason why we need a special method, PreventDefaultBeforeDispatch() is needed for setting only mDefaultPrevented to true. Review commit: https://reviewboard.mozilla.org/r/41241/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41241/

Review commit: https://reviewboard.mozilla.org/r/41243/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41243/

Review commit: https://reviewboard.mozilla.org/r/41245/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41245/

Review commit: https://reviewboard.mozilla.org/r/41247/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41247/

(In reply to Olli Pettay [:smaug] from comment #2) > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #1) > > Should be moved: > > > > StopPropagation() > > StopImmediatePropagation() > > StopCrossProcessForwarding() > > PreventDefault() > > > These look fine > > > DefaultPrevented() > > DefaultPreventedByContent() > > DefaultPreventedByOnlyChrome() > > IsTrusted() > I guess these too, though I'm not sure whether > any widget level code should care about DefaultPreventedByContent() or > DefaultPreventedByOnlyChrome() I stopped creating DefaultPreventedByOnlyChrome() because it's not useful. However, I added PropagationStopped() this is useful when you need to check where/when the event's propagation stopped. (E.g., inserting MOZ_ASSERT() with checking the members, e.g., MOZ_ASSERT(!(mMessage == eKeyDown && mKeyValue.EqualsLiteral("t")))

Comment on attachment 8732590 [details] MozReview Request: Bug 1256589 part.1 Move the implementation of StopPropagation() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41235/#review37997

Comment on attachment 8732591 [details] MozReview Request: Bug 1256589 part.2 Move the implementation of StopImmediatePropagation() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41237/#review37999

Comment on attachment 8732592 [details] MozReview Request: Bug 1256589 part.3 Move the implementation of StopCrossProcessForwarding() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41239/#review38001 ::: dom/ipc/TabParent.cpp:2111 (Diff revision 1) > TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event) > { > NS_ENSURE_TRUE(mFrameElement, true); > > WidgetKeyboardEvent localEvent(event); > // Set mNoCrossProcessBoundaryForwarding to avoid this event from want to update this comment too. It talks about mNoCrossProcessBoundaryForwarding

Comment on attachment 8732594 [details] MozReview Request: Bug 1256589 part.5 Add DefaultPrevented() and DefaultPreventedByContent() to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41243/#review38007 But not a big thing. I would probably add an assertion. ::: widget/BasicEvents.h:173 (Diff revision 1) > + { > + return mDefaultPrevented; > + } > + inline bool DefaultPreventedByContent() const > + { > + return DefaultPrevented() && mDefaultPreventedByContent; Why do we need to call DefaultPrevented() here? mDefaultPreventedByContent should never true if mDefaultPrevented is false. We could even assert about that.

Comment on attachment 8732595 [details] MozReview Request: Bug 1256589 part.6 Move the implementation of IsTrusted() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41245/#review38013

Comment on attachment 8732596 [details] MozReview Request: Bug 1256589 part.7 Add PropagationStopped() to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41247/#review38015

Comment on attachment 8732593 [details] MozReview Request: Bug 1256589 part.4 Move the implementation of PreventDefault() and add PreventDefaultBeforeDispatch() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41241/#review38017 r- because of that presshell part. I don't understand the change to mDefaultPreventedByChrome usage there. ::: dom/events/EventListenerManager.cpp:1197 (Diff revision 1) > //Set the value of the internal PreventDefault flag properly based on aEventStatus > - if (*aEventStatus == nsEventStatus_eConsumeNoDefault) { > - aEvent->mFlags.mDefaultPrevented = true; > + if (!aEvent->mFlags.mDefaultPrevented && > + *aEventStatus == nsEventStatus_eConsumeNoDefault) { > + // Assume that if only aEventStatus claims that the event has already been > + // consumed, the consumer is default event handler. > + aEvent->PreventDefault(); So this changes mDefaultPreventedByChrome handling a bit since the old code wouldn't have set it, but this new one does set it. I think that is fine. ::: dom/events/EventStateManager.cpp:3185 (Diff revision 1) > // if the wheel event is synthesized from a Mac trackpad or other device > // that can generate additional momentum events, then we should allow > // APZ to handle it, because it will track the velocity and predicted > // destination from the momentum. > if (wheelEvent->mFlags.mHandledByAPZ) { > - wheelEvent->mFlags.mDefaultPrevented = true; > + wheelEvent->PreventDefault(); Same also here. ::: dom/events/IMEContentObserver.cpp:827 (Diff revision 1) > return false; > } > > bool consumed = (rv == NS_SUCCESS_EVENT_CONSUMED); > - aMouseEvent->mFlags.mDefaultPrevented = consumed; > + if (consumed) { > + aMouseEvent->PreventDefault(); and here ::: layout/base/nsPresShell.cpp:7936 (Diff revision 1) > if (root && root->GetFullscreenElement()) { > // Prevent default action on ESC key press when exiting > // DOM fullscreen mode. This prevents the browser ESC key > // handler from stopping all loads in the document, which > // would cause <video> loads to stop. > - aEvent->mFlags.mDefaultPrevented = true; > + // XXX We need to claim the Escape key event which will be ah, right, this is about the case when we need to somehow detect two esc presses or something. yeah, hard to read code. We could possibly remove some of this stuff to reserved XUL key handlers now that it may override content handlers. maybe. But anyhow, shouldn't you call PreventDefaultBeforeDispatch() here. ::: layout/base/nsPresShell.cpp:7961 (Diff revision 1) > nsCOMPtr<nsIDocument> pointerLockedDoc = > do_QueryReferent(EventStateManager::sPointerLockedDoc); > if (!mIsLastChromeOnlyEscapeKeyConsumed && pointerLockedDoc) { > - aEvent->mFlags.mDefaultPrevented = true; > + // XXX See above comment to understand the reason why this needs > + // to claim that the Escape key event is consumed by content > + // even though it will be dispatched only into chrome. and here.

https://reviewboard.mozilla.org/r/41239/#review38001 > want to update this comment too. It talks about mNoCrossProcessBoundaryForwarding > TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event) > { > NS_ENSURE_TRUE(mFrameElement, true); > > WidgetKeyboardEvent localEvent(event); > // Mark the event as not to be dispatched to remote process again. > localEvent.StopCrossProcessForwarding(); updated.

https://reviewboard.mozilla.org/r/41243/#review38007 > Why do we need to call DefaultPrevented() here? > mDefaultPreventedByContent should never true if mDefaultPrevented is false. We could even assert about that. > inline bool DefaultPreventedByContent() const > { > MOZ_ASSERT(!mDefaultPreventedByContent || DefaultPrevented()); > return mDefaultPreventedByContent; > }

https://reviewboard.mozilla.org/r/41241/#review38017 Looks like mDefaultPreventedByChrome is hacky... The only use case of thi is, checking the Escape keyboard event status in fullscreen mode. I don't understand why PressShell needs to call PreventDefaultBeforeDispatch() in such case. Can it replaced with setting mOnlySystemGroupDispatchInContent to true without marking the event's default prevented? > ah, right, this is about the case when we need to somehow detect two esc presses or something. yeah, hard to read code. > > We could possibly remove some of this stuff to reserved XUL key handlers now that it may override content handlers. maybe. > > > But anyhow, shouldn't you call PreventDefaultBeforeDispatch() here. Yeah, indeed. I just forgot to update PresShell. # I wonder, cannot we remove nsEventStatus completely if we can drop nsEventStatus_eConsumeDoDefault?

Comment on attachment 8732590 [details] MozReview Request: Bug 1256589 part.1 Move the implementation of StopPropagation() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41235/diff/1-2/

Comment on attachment 8732591 [details] MozReview Request: Bug 1256589 part.2 Move the implementation of StopImmediatePropagation() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41237/diff/1-2/

Comment on attachment 8732592 [details] MozReview Request: Bug 1256589 part.3 Move the implementation of StopCrossProcessForwarding() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41239/diff/1-2/

Comment on attachment 8732593 [details] MozReview Request: Bug 1256589 part.4 Move the implementation of PreventDefault() and add PreventDefaultBeforeDispatch() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41241/diff/1-2/

Comment on attachment 8732594 [details] MozReview Request: Bug 1256589 part.5 Add DefaultPrevented() and DefaultPreventedByContent() to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41243/diff/1-2/

Comment on attachment 8732595 [details] MozReview Request: Bug 1256589 part.6 Move the implementation of IsTrusted() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41245/diff/1-2/

Comment on attachment 8732596 [details] MozReview Request: Bug 1256589 part.7 Add PropagationStopped() to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41247/diff/1-2/

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #25) > https://reviewboard.mozilla.org/r/41241/#review38017 > > Looks like mDefaultPreventedByChrome is hacky... The only use case of thi > is, checking the Escape keyboard event status in fullscreen mode. I don't > understand why PressShell needs to call PreventDefaultBeforeDispatch() in > such case. > > Can it replaced with setting mOnlySystemGroupDispatchInContent to true > without marking the event's default prevented? Well, I wouldn't change anything there in this bug, just to keep the scope of this bug clear. > # I wonder, cannot we remove nsEventStatus completely if we can drop > nsEventStatus_eConsumeDoDefault? I'd love to see nsEventStatus to go away. I think there is even an old bug open to do that, but when someone (was it jwatt) tried to fix it, it was at that time still way too large task and complicated.

Comment on attachment 8732593 [details] MozReview Request: Bug 1256589 part.4 Move the implementation of PreventDefault() and add PreventDefaultBeforeDispatch() from dom::Event to WidgetEvent r=smaug https://reviewboard.mozilla.org/r/41241/#review38159 ::: widget/BasicEvents.h:149 (Diff revision 2) > } > inline void StopCrossProcessForwarding() > { > mNoCrossProcessBoundaryForwarding = true; > } > + inline void PreventDefault(bool aCalledByDefaultHandler = true) Another option would be to have some enum which can be passed to PreventDefault() and that defaults to ePreventedByDefaultHandler or some such. enum PreventDefaultOptions { eBeforeDispatch, eDefaultPreventedByContent, ePreventedByDefaultHandler }; But I guess that doesn't really make things much easier to understand.

(In reply to Olli Pettay [:smaug] from comment #34) > Comment on attachment 8732593 [details] > MozReview Request: Bug 1256589 part.4 Move the implementation of > PreventDefault() and add PreventDefaultBeforeDispatch() from dom::Event to > WidgetEvent r?smaug > > https://reviewboard.mozilla.org/r/41241/#review38159 > > ::: widget/BasicEvents.h:149 > (Diff revision 2) > > } > > inline void StopCrossProcessForwarding() > > { > > mNoCrossProcessBoundaryForwarding = true; > > } > > + inline void PreventDefault(bool aCalledByDefaultHandler = true) > > Another option would be to have some enum which can be passed to > PreventDefault() and that defaults to ePreventedByDefaultHandler or some > such. > enum PreventDefaultOptions > { > eBeforeDispatch, > eDefaultPreventedByContent, > ePreventedByDefaultHandler > }; > > But I guess that doesn't really make things much easier to understand. Yeah. Fortunately, the only user of eDefaultPreventedByContent is Event.cpp. And eBeforeDispatch users are a few. So, I think that it's enough to separate the method only for eBeforeDispatch because it allows developers to search enough easy. (In reply to Olli Pettay [:smaug] from comment #33) > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #25) > > # I wonder, cannot we remove nsEventStatus completely if we can drop > > nsEventStatus_eConsumeDoDefault? > I'd love to see nsEventStatus to go away. I think there is even an old bug > open to do that, but when someone > (was it jwatt) tried to fix it, it was at that time still way too large task > and complicated. Found bug 331630. I'll try to do it when I have much time.

Comment on attachment 8732590 [details] MozReview Request: Bug 1256589 part.1 Move the implementation of StopPropagation() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41235/diff/2-3/

Comment on attachment 8732591 [details] MozReview Request: Bug 1256589 part.2 Move the implementation of StopImmediatePropagation() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41237/diff/2-3/

Comment on attachment 8732592 [details] MozReview Request: Bug 1256589 part.3 Move the implementation of StopCrossProcessForwarding() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41239/diff/2-3/

Comment on attachment 8732593 [details] MozReview Request: Bug 1256589 part.4 Move the implementation of PreventDefault() and add PreventDefaultBeforeDispatch() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41241/diff/2-3/

Comment on attachment 8732594 [details] MozReview Request: Bug 1256589 part.5 Add DefaultPrevented() and DefaultPreventedByContent() to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41243/diff/2-3/

Comment on attachment 8732595 [details] MozReview Request: Bug 1256589 part.6 Move the implementation of IsTrusted() from dom::Event to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41245/diff/2-3/

Comment on attachment 8732596 [details] MozReview Request: Bug 1256589 part.7 Add PropagationStopped() to WidgetEvent r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41247/diff/2-3/

https://hg.mozilla.org/integration/mozilla-inbound/rev/87b0d12a4d1c https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b19f55d829 https://hg.mozilla.org/integration/mozilla-inbound/rev/93b3ea492ea7 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc36cc31ae9 https://hg.mozilla.org/integration/mozilla-inbound/rev/0881a3d1697a https://hg.mozilla.org/integration/mozilla-inbound/rev/927075631583 https://hg.mozilla.org/integration/mozilla-inbound/rev/eb62677979ba

https://hg.mozilla.org/mozilla-central/rev/87b0d12a4d1c https://hg.mozilla.org/mozilla-central/rev/e8b19f55d829 https://hg.mozilla.org/mozilla-central/rev/93b3ea492ea7 https://hg.mozilla.org/mozilla-central/rev/bfc36cc31ae9 https://hg.mozilla.org/mozilla-central/rev/0881a3d1697a https://hg.mozilla.org/mozilla-central/rev/927075631583 https://hg.mozilla.org/mozilla-central/rev/eb62677979ba