Mason Freed (Gerrit)

unread,

Jun 18, 2025, 5:03:33 PMJun 18

to Daniil Sakhapov, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Attention needed from Daniil Sakhapov

Mason Freed voted and added 5 comments

Votes added by Mason Freed

Code-Review

+1

5 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Mason Freed . resolved

First, sorry for the huge delay - it's been busy.

LGTM, just a few small questions and nits.

File third_party/blink/renderer/core/dom/events/event.h

Line 150, Patchset 2 (Latest): EventTarget* RawTarget() const { return target_.Get(); }

Mason Freed . unresolved

The `Raw` prefix - I'm not sure I love it. I was thinking I wanted to say `TargetIncludingPseudos` or something, but that's pretty long. I guess it's ok. Just wondering if there were any other options considered?

File third_party/blink/renderer/core/dom/events/event.cc

Line 62, Patchset 2 (Latest): // For ::scroll-marker, the target should be the ultimate originating element

Mason Freed . unresolved

Since this function might be kind of hot on benchmarks, perhaps you could short-circuit the faster case right here?

```
if (!node || !node->IsPseudoElement()) {
  return target;
}
```

Line 74, Patchset 2 (Latest): << "target can't be pseudo element! found " << node;

Mason Freed . unresolved

nit: `can't be a pseudo`

Line 315, Patchset 2 (Latest):

  EventTarget* retarget_against =
      raw_current_target ? raw_current_target : RawTarget();

Mason Freed . unresolved

nit:

```
if (!retarget_against) {
  retarget_against = RawTarget();
}
```

Open in Gerrit

Related details

Attention is currently required from:

Daniil Sakhapov

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniil Sakhapov (Gerrit)

unread,

Jun 19, 2025, 4:06:16 AMJun 19

to Mason Freed, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Attention needed from Mason Freed

Daniil Sakhapov voted and added 5 comments

Votes added by Daniil Sakhapov

Commit-Queue

+1

5 comments

Patchset-level comments

File-level comment, Patchset 2:

Mason Freed . resolved

First, sorry for the huge delay - it's been busy.
LGTM, just a few small questions and nits.

Daniil Sakhapov

Thanks for taking a look!

File third_party/blink/renderer/core/dom/events/event.h

Line 150, Patchset 2: EventTarget* RawTarget() const { return target_.Get(); }

Mason Freed . unresolved

The `Raw` prefix - I'm not sure I love it. I was thinking I wanted to say `TargetIncludingPseudos` or something, but that's pretty long. I guess it's ok. Just wondering if there were any other options considered?

Daniil Sakhapov

UnretargetedTarget? :) what about OriginalTarget?

File third_party/blink/renderer/core/dom/events/event.cc

Line 62, Patchset 2: // For ::scroll-marker, the target should be the ultimate originating element

Mason Freed . resolved

Since this function might be kind of hot on benchmarks, perhaps you could short-circuit the faster case right here?
```
if (!node || !node->IsPseudoElement()) {
  return target;
}
```

Daniil Sakhapov

Done

Line 74, Patchset 2: << "target can't be pseudo element! found " << node;

Mason Freed . resolved

nit: `can't be a pseudo`

Daniil Sakhapov

Done

Line 315, Patchset 2:

  EventTarget* retarget_against =
      raw_current_target ? raw_current_target : RawTarget();

Mason Freed . resolved

nit:

```
if (!retarget_against) {
  retarget_against = RawTarget();
}
```

Daniil Sakhapov

Done

Open in Gerrit

Related details

Attention is currently required from:

Mason Freed

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mason Freed (Gerrit)

unread,

Jun 20, 2025, 6:06:14 PMJun 20

to Daniil Sakhapov, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Attention needed from Daniil Sakhapov

Mason Freed voted and added 2 comments

Votes added by Mason Freed

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Mason Freed . resolved

Still LGTM

File third_party/blink/renderer/core/dom/events/event.h

Line 150, Patchset 2: EventTarget* RawTarget() const { return target_.Get(); }

Mason Freed . unresolved

The `Raw` prefix - I'm not sure I love it. I was thinking I wanted to say `TargetIncludingPseudos` or something, but that's pretty long. I guess it's ok. Just wondering if there were any other options considered?

Daniil Sakhapov

UnretargetedTarget? :) what about OriginalTarget?

Mason Freed

Of those two, I think I prefer `RawTarget`. It's ok - let's just leave it as you have it here.

Open in Gerrit

Related details

Attention is currently required from:

Daniil Sakhapov

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniil Sakhapov (Gerrit)

unread,

Jun 23, 2025, 3:05:54 AMJun 23

to Mason Freed, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Daniil Sakhapov voted and added 1 comment

Votes added by Daniil Sakhapov

Commit-Queue

+2

1 comment

File third_party/blink/renderer/core/dom/events/event.h

Line 150, Patchset 2: EventTarget* RawTarget() const { return target_.Get(); }

Mason Freed . resolved

The `Raw` prefix - I'm not sure I love it. I was thinking I wanted to say `TargetIncludingPseudos` or something, but that's pretty long. I guess it's ok. Just wondering if there were any other options considered?

Daniil Sakhapov

UnretargetedTarget? :) what about OriginalTarget?

Mason Freed

Of those two, I think I prefer `RawTarget`. It's ok - let's just leave it as you have it here.

Daniil Sakhapov

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 23, 2025, 3:56:04 AMJun 23

to Daniil Sakhapov, Mason Freed, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Don't expose pseudo element to web via event.target and .currentTarget

The only possible cases now are ::scroll-marker and ::scroll-button().

Also, this CL changes internal call sites of target() to RawTarget(),
as internally we need to work with pseudo elements.

Bug: 420463805

Change-Id: Ica69a3de925d04524cad97c8fd75657c71515d1f

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6633705

Reviewed-by: Mason Freed <mas...@chromium.org>

Commit-Queue: Daniil Sakhapov <sakh...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1477142}

Files:

M third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc
M third_party/blink/renderer/bindings/core/v8/js_event_handler.cc
M third_party/blink/renderer/core/dom/events/event.cc
M third_party/blink/renderer/core/dom/events/event.h
M third_party/blink/renderer/core/dom/events/event_dispatcher.cc
M third_party/blink/renderer/core/dom/events/event_queue.cc
M third_party/blink/renderer/core/dom/events/scoped_event_queue.cc
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/scripted_animation_controller.cc
M third_party/blink/renderer/core/dom/scroll_button_pseudo_element.cc
M third_party/blink/renderer/core/dom/scroll_marker_pseudo_element.cc
M third_party/blink/renderer/core/editing/commands/insert_commands.cc
M third_party/blink/renderer/core/editing/editor.cc
M third_party/blink/renderer/core/editing/editor_key_bindings.cc
M third_party/blink/renderer/core/events/mouse_event.cc
M third_party/blink/renderer/core/html/forms/html_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_form_element.cc
M third_party/blink/renderer/core/html/forms/html_label_element.cc
M third_party/blink/renderer/core/html/forms/range_input_type.cc
M third_party/blink/renderer/core/html/forms/select_type.cc
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/html/html_summary_element.cc
M third_party/blink/renderer/core/html/media/media_document.cc
M third_party/blink/renderer/core/input/event_handler.cc
M third_party/blink/renderer/core/page/context_menu_controller.cc
M third_party/blink/renderer/core/page/focusgroup_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation_controller.cc
M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
M third_party/blink/renderer/core/timing/window_performance.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_text_track_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_track_selector_list_element.cc
M third_party/blink/renderer/modules/media_controls/media_controls_shared_helper.cc
A third_party/blink/web_tests/external/wpt/css/css-overflow/scroll-button-event-target.html
A third_party/blink/web_tests/external/wpt/css/css-overflow/scroll-marker-event-target.html

Change size: L

Delta: 36 files changed, 257 insertions(+), 71 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Mason Freed

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Blink W3C Test Autoroller (Gerrit)

unread,

Jun 23, 2025, 4:45:47 AMJun 23

to Daniil Sakhapov, Chromium LUCI CQ, Mason Freed, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Message from Blink W3C Test Autoroller

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53317

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Daniil Sakhapov (Gerrit)

unread,

Jun 24, 2025, 10:06:50 AMJun 24

to Chromium LUCI CQ, Blink W3C Test Autoroller, Mason Freed, AyeAye, Alexis Menard, David Bokan, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, chromium...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Daniil Sakhapov has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Daniil Sakhapov (Gerrit)

unread,

Jun 26, 2025, 8:50:03 AM (13 days ago) Jun 26

to Rune Lillesveen, Mason Freed, Chromium LUCI CQ, Alexis Menard, David Bokan, chromium...@chromium.org, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, dtapuska+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org

Attention needed from Mason Freed and Rune Lillesveen

Daniil Sakhapov added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Daniil Sakhapov . unresolved

This is a retry of https://chromium-review.googlesource.com/c/chromium/src/+/6633705, but this time targets are cached at setter, instead of dynamically being calculated at getter to avoid detached pseudos crashes.
Also, this time there is no NOTREACHED on pseudos that are not ::scroll-button or ::scroll-marker. I don't know what other pseudos we potentially expose to the web now, but I'll investiage it.

Open in Gerrit

Related details

Attention is currently required from:

Mason Freed
Rune Lillesveen

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rune Lillesveen (Gerrit)

unread,

Jun 26, 2025, 10:39:34 AM (13 days ago) Jun 26

to Daniil Sakhapov, Rune Lillesveen, Mason Freed, Chromium LUCI CQ, Alexis Menard, David Bokan, chromium...@chromium.org, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, dtapuska+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org

Attention needed from Daniil Sakhapov and Mason Freed

Rune Lillesveen added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Daniil Sakhapov . unresolved

This is a retry of https://chromium-review.googlesource.com/c/chromium/src/+/6633705, but this time targets are cached at setter, instead of dynamically being calculated at getter to avoid detached pseudos crashes.
Also, this time there is no NOTREACHED on pseudos that are not ::scroll-button or ::scroll-marker. I don't know what other pseudos we potentially expose to the web now, but I'll investiage it.

Rune Lillesveen

Could you upload the previous attempt first, and then apply the latest patch set on top? I suspect that it would be easier to review the diff.

Open in Gerrit

Related details

Attention is currently required from:

Daniil Sakhapov
Mason Freed

satisfied_requirement

unsatisfied_requirement

open

diffy

Mason Freed (Gerrit)

unread,

Jun 26, 2025, 4:45:04 PM (12 days ago) Jun 26

to Daniil Sakhapov, Rune Lillesveen, Chromium LUCI CQ, Alexis Menard, David Bokan, chromium...@chromium.org, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, dtapuska+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org

Attention needed from Daniil Sakhapov

Mason Freed added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Daniil Sakhapov . unresolved

This is a retry of https://chromium-review.googlesource.com/c/chromium/src/+/6633705, but this time targets are cached at setter, instead of dynamically being calculated at getter to avoid detached pseudos crashes.
Also, this time there is no NOTREACHED on pseudos that are not ::scroll-button or ::scroll-marker. I don't know what other pseudos we potentially expose to the web now, but I'll investiage it.

Rune Lillesveen

Could you upload the previous attempt first, and then apply the latest patch set on top? I suspect that it would be easier to review the diff.

Mason Freed

+1, thank you!

Open in Gerrit

Related details

Attention is currently required from:

Daniil Sakhapov

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniil Sakhapov (Gerrit)

unread,

Jun 27, 2025, 7:24:45 AM (12 days ago) Jun 27

to Rune Lillesveen, Mason Freed, Chromium LUCI CQ, Alexis Menard, David Bokan, chromium...@chromium.org, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, Alex Keng, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, dom+...@chromium.org, dtapuska+...@chromium.org, feature-me...@chromium.org, hiroshig...@chromium.org, kouhei...@chromium.org, speed-metrics...@chromium.org, steimel+watch...@chromium.org

Daniil Sakhapov abandoned this change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Don't expose pseudo element to web via event.target and .currentTarget [chromium/src : main]

Mason Freed (Gerrit)

Mason Freed voted and added 5 comments

Votes added by Mason Freed

5 comments

Related details

Daniil Sakhapov (Gerrit)

Daniil Sakhapov voted and added 5 comments

Votes added by Daniil Sakhapov

5 comments

Related details

Mason Freed (Gerrit)

Mason Freed voted and added 2 comments

Votes added by Mason Freed

2 comments

Related details

Daniil Sakhapov (Gerrit)

Daniil Sakhapov voted and added 1 comment

Votes added by Daniil Sakhapov

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Blink W3C Test Autoroller (Gerrit)

Message from Blink W3C Test Autoroller

Related details

Daniil Sakhapov (Gerrit)

Daniil Sakhapov has created a revert of this change

Related details

Daniil Sakhapov (Gerrit)

Daniil Sakhapov added 1 comment

Related details

Rune Lillesveen (Gerrit)

Rune Lillesveen added 1 comment

Related details

Mason Freed (Gerrit)

Mason Freed added 1 comment

Related details

Daniil Sakhapov (Gerrit)

Daniil Sakhapov abandoned this change

Related details