2035992 - Add an API to get proper type of event target from `nsIFrame`

Status: RESOLVED FIXED

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

Description

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

Currently, nsIFrame::GetContentForEvent() return explicit event target if an event is handled by the frame. I.e., it may return a Text even if the given event should target an Element node. Some callers use the event target for such event as-is and the others retrieve an element by themselves (and some of them use wrong way). So, there should be a method like nsIFrame::GetEventTargetContent().

(Before the national holiday week in Japan, I have a spare time, so, I'm trying to fix this now.)

Comment 1

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

Attachment: Bug 2035992 - Add `nsIFrame::GetEventTargetContent()` to get proper event target from the given event r=smaug!,emilio!

Most event targets an element node even if the input occurs on a Text.
EventStateManager, PresShell, PresShell::EventHandler, etc use
the result of nsIFrame::GetContentForEvent result as-is or looking
for proper ancestor element by the caller selves.

So, I think there should be GetEventTargetContent() which solves the
target node kind automatically and in the same way. And also this patch
renames nsIFrame::GetContentForEvent to
nsIFrame::GetEventTargetExplicitContent for consistency with
Event::mExplicitOriginalTarget.

Finally, this patch changes the callers which retrieves a parent
Element if the result of GetContentForEvent is not an Element to
use the new GetEventTargetContent.

The following patches make each caller of GetContentForEvent marked
with "FIXME" comment fix one by one.

Comment 2

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

Attachment: Bug 2035992 - Make `EventHandler::EventTargetData::SetContentForEventFromFrame` use `nsIFrame::GetEventTargetContent` r=smaug!

Comment 3

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

Attachment: Bug 2035992 - Make `PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent` use `nsIFrame::GetEventTargetContent` to set `WidgetGUIEvent::mTarget` r=smaug!

Comment 4

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

Attachment: Bug 2035992 - Make `PointerEventHandler::DispatchPointerEventWithTarget` use `nsIFrame::GetEventTargetContent` to consider the pointer event target r=smaug!

Comment 5

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

Attachment: Bug 2035992 - Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the first candidate of new focus element at mouse button down r=smaug!

Comment 6

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

Attachment: Bug 2035992 - Make `EventStateManager::GenerateDragDropEnterExit` use `nsIFrame::GetEventTargetContent` to consider `eDragExit` and `eDragLeave` event target r=smaug!

Comment 7

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

Attachment: Bug 2035992 - Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the hovered content r=smaug!

Comment 8

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

Attachment: Bug 2035992 - Add `EventStateManager::GetEventTargetContent` and `PresShell::GetEventTargetContent` r=smaug!

They have only GetExplicitEventTargetContent and some callers may want
to get proper target Element. Therefore, we should add them and the
common logic should be shared.

nsListControlFrame::GetIndexFromEvent looks for an inclusive ancestor
<option>. So, it can start the loop from its inclusive ancestor
element of the explicit event target so that it can use the new API.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/84f304eb7022
https://hg.mozilla.org/integration/autoland/rev/2ce83a14e8fd
Add `nsIFrame::GetEventTargetContent()` to get proper event target from the given event r=smaug,emilio,layout-reviewers
https://github.com/mozilla-firefox/firefox/commit/a46e328cae77
https://hg.mozilla.org/integration/autoland/rev/8ec99dbfa3eb
Make `EventHandler::EventTargetData::SetContentForEventFromFrame` use `nsIFrame::GetEventTargetContent` r=smaug
https://github.com/mozilla-firefox/firefox/commit/95a8aec38456
https://hg.mozilla.org/integration/autoland/rev/507681433e46
Make `PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent` use `nsIFrame::GetEventTargetContent` to set `WidgetGUIEvent::mTarget` r=smaug
https://github.com/mozilla-firefox/firefox/commit/fa2354f5f8bb
https://hg.mozilla.org/integration/autoland/rev/c3ea91ee9d64
Make `PointerEventHandler::DispatchPointerEventWithTarget` use `nsIFrame::GetEventTargetContent` to consider the pointer event target r=smaug
https://github.com/mozilla-firefox/firefox/commit/7163909ad8bd
https://hg.mozilla.org/integration/autoland/rev/21d73cd654a9
Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the first candidate of new focus element at mouse button down r=smaug
https://github.com/mozilla-firefox/firefox/commit/adc2bf108109
https://hg.mozilla.org/integration/autoland/rev/ec5423d5cac6
Make `EventStateManager::GenerateDragDropEnterExit` use `nsIFrame::GetEventTargetContent` to consider `eDragExit` and `eDragLeave` event target r=smaug
https://github.com/mozilla-firefox/firefox/commit/65dfa9f9845e
https://hg.mozilla.org/integration/autoland/rev/5d1fe971b0e5
Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the hovered content r=smaug
https://github.com/mozilla-firefox/firefox/commit/c1105686daa5
https://hg.mozilla.org/integration/autoland/rev/cedacfffa537
Add `EventStateManager::GetEventTargetContent` and `PresShell::GetEventTargetContent` r=smaug

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/37fe53f379f5
https://hg.mozilla.org/integration/autoland/rev/5fea499bd26d
Revert "Bug 2035992 - Add `EventStateManager::GetEventTargetContent` and `PresShell::GetEventTargetContent` r=smaug" for causing build bustages in EventStateManager.cpp.

Comment 11

[Serban Stanca [:SerbanS]]

Reverted this because it was causing build bustages in EventStateManager.cpp.

• Revert link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/dom/events/EventStateManager.cpp:X:29: error: no member named 'GetContentForEvent' in 'nsIFrame'

Comment 12

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

Wow, it's macOS specific path...

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b77593c2f7a2
https://hg.mozilla.org/integration/autoland/rev/0e6e677eb115
Add `nsIFrame::GetEventTargetContent()` to get proper event target from the given event r=smaug,emilio,layout-reviewers
https://github.com/mozilla-firefox/firefox/commit/03c09548c4a3
https://hg.mozilla.org/integration/autoland/rev/e18c8223c6f9
Make `EventHandler::EventTargetData::SetContentForEventFromFrame` use `nsIFrame::GetEventTargetContent` r=smaug
https://github.com/mozilla-firefox/firefox/commit/f5281398f280
https://hg.mozilla.org/integration/autoland/rev/76df321a398a
Make `PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent` use `nsIFrame::GetEventTargetContent` to set `WidgetGUIEvent::mTarget` r=smaug
https://github.com/mozilla-firefox/firefox/commit/fb05f23ec646
https://hg.mozilla.org/integration/autoland/rev/160817eec313
Make `PointerEventHandler::DispatchPointerEventWithTarget` use `nsIFrame::GetEventTargetContent` to consider the pointer event target r=smaug
https://github.com/mozilla-firefox/firefox/commit/0ce37dcbe8b8
https://hg.mozilla.org/integration/autoland/rev/3c53869e5ecf
Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the first candidate of new focus element at mouse button down r=smaug
https://github.com/mozilla-firefox/firefox/commit/9865841bcc86
https://hg.mozilla.org/integration/autoland/rev/121df849d148
Make `EventStateManager::GenerateDragDropEnterExit` use `nsIFrame::GetEventTargetContent` to consider `eDragExit` and `eDragLeave` event target r=smaug
https://github.com/mozilla-firefox/firefox/commit/b8c387296cc3
https://hg.mozilla.org/integration/autoland/rev/d36e0da0472b
Make `EventStateManager::PostHandleEvent` use `nsIFrame::GetEventTargetContent` to consider the hovered content r=smaug
https://github.com/mozilla-firefox/firefox/commit/a7b798c8d294
https://hg.mozilla.org/integration/autoland/rev/73f6fdd33abd
Add `EventStateManager::GetEventTargetContent` and `PresShell::GetEventTargetContent` r=smaug

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/0e6e677eb115
https://hg.mozilla.org/mozilla-central/rev/e18c8223c6f9
https://hg.mozilla.org/mozilla-central/rev/76df321a398a
https://hg.mozilla.org/mozilla-central/rev/160817eec313
https://hg.mozilla.org/mozilla-central/rev/3c53869e5ecf
https://hg.mozilla.org/mozilla-central/rev/121df849d148
https://hg.mozilla.org/mozilla-central/rev/d36e0da0472b
https://hg.mozilla.org/mozilla-central/rev/73f6fdd33abd