Fix <label> default action when clicked button is removed [chromium/src : main]
0 views
Skip to first unread message
Jinho Bang (Gerrit)
Jan 3, 2026, 8:54:42 PM (7 days ago) Jan 3
to YeongHan Kim, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from YeongHan Kim
Jinho Bang added 4 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Jinho Bang . resolved
Looks good to me. I'm not a DOM expert, so the owners might have different feedback. I've left a few minor comments for now.
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 167, Patchset 4 (Latest):
if (!evt.HasEventPath()) {
return false;
}Jinho Bang . unresolved
Isn't HasEventPath() always true at this time? If so, can we use DCHECK instead of if-statement?
Line 170, Patchset 4 (Latest):
const EventPath& event_path = evt.GetEventPath();
for (wtf_size_t i = 0; i < event_path.size(); ++i) {
Node& node_in_path = event_path[i].GetNode();Jinho Bang . unresolved
We could use `NodeEventContexts()` here as follows:
```
for (auto& context : evt.GetEventPath().NodeEventContexts()) {
Node& node = context.GetNode();
```
Line 181, Patchset 4 (Latest):
return false;Jinho Bang . unresolved
I think this is unreachable. Is my understanding correct?
Then, can we add `NOTREACHED()` macro here?
Related details
Attention is currently required from:
- YeongHan Kim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jan 3, 2026, 8:57:09 PM (7 days ago) Jan 3
to YeongHan Kim, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from YeongHan Kim
Jinho Bang added 2 comments![]( "Open in Gerrit")
File third_party/blink/web_tests/external/wpt/dom/events/Label-default-action.html
Line 1, Patchset 4 (Latest):
<!DOCTYPE html>Jinho Bang . unresolved
I think this file name should be lowercase.
https://web-platform-tests.org/writing-tests/general-guidelines.html
YeongHan Kim (Gerrit)
Jan 4, 2026, 6:34:59 AM (6 days ago) Jan 4
to Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang
YeongHan Kim added 6 comments![]( "Open in Gerrit")
Patchset-level comments
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Isn't HasEventPath() always true at this time? If so, can we use DCHECK instead of if-statement?
YeongHan Kim
Done!
Line 170, Patchset 4:
const EventPath& event_path = evt.GetEventPath();
for (wtf_size_t i = 0; i < event_path.size(); ++i) {
Node& node_in_path = event_path[i].GetNode();Jinho Bang . resolved
We could use `NodeEventContexts()` here as follows:
```
for (auto& context : evt.GetEventPath().NodeEventContexts()) {
Node& node = context.GetNode();
```
YeongHan Kim
Oh, that looks like a better approach. Thank you!
I think this is unreachable. Is my understanding correct?
Then, can we add `NOTREACHED()` macro here?
YeongHan Kim
You’re right. I’ve applied the changes!
File third_party/blink/web_tests/external/wpt/dom/events/Label-default-action.html
I think this file name should be lowercase.
https://web-platform-tests.org/writing-tests/general-guidelines.html
YeongHan Kim
Done!
Please remove unnecessary blank lines
Attention is currently required from:
- Jinho Bang
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jan 4, 2026, 7:01:04 PM (6 days ago) Jan 4
to YeongHan Kim, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from YeongHan Kim
Jinho Bang added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 170, Patchset 6 (Latest):
if (&node == this) {Jinho Bang . unresolved
nit: `node.IsSameNode(this);` is better for readability
Related details
Attention is currently required from:
- YeongHan Kim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
Jan 5, 2026, 9:36:02 AM (5 days ago) Jan 5
to Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang
YeongHan Kim added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 7 (Latest):
YeongHan Kim . resolved
Thank you for your review. I’ve applied your comment!
File third_party/blink/renderer/core/html/forms/html_label_element.cc
nit: `node.IsSameNode(this);` is better for readability
Attention is currently required from:
- Jinho Bang
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
Jan 5, 2026, 10:26:22 AM (5 days ago) Jan 5
to Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and Joey Arhar
YeongHan Kim added 2 comments![]( "Open in Gerrit")
Patchset-level comments
YeongHan Kim . resolved
Please take a look. Thank you!
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 168, Patchset 7 (Latest):
for (auto& context : evt.GetEventPath().NodeEventContexts()) {YeongHan Kim . resolved
Since the `EventPath` is captured at the start of event dispatch, it already reflects the original DOM structure, so no extra handling is needed to preserve the `IsInInteractiveContent()` value.
Related details
Attention is currently required from:
- Jinho Bang
- Joey Arhar
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Jan 6, 2026, 4:58:41 PM (4 days ago) Jan 6
to YeongHan Kim, Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and YeongHan Kim
Joey Arhar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
3 comments
File third_party/blink/renderer/core/html/forms/html_label_element.h
Line 46, Patchset 7 (Latest):
bool IsInInteractiveContent(Node*) const;Joey Arhar . unresolved
want to add a TODO to remove this when the flag is removed?
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 170, Patchset 7 (Latest):
if (node.isSameNode(this)) {Joey Arhar . unresolved
I didn't know this method exists, but want to just do `&node == this` instead?
File third_party/blink/web_tests/external/wpt/dom/events/label-default-action.html
Line 2, Patchset 7 (Latest):
<title>Label default action with interactive content</title>Joey Arhar . unresolved
want to add
```
<link rel=help href="https://issues.chromium.org/issues/452084024">
```
Related details
Attention is currently required from:
- Jinho Bang
- YeongHan Kim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
Jan 7, 2026, 10:53:44 AM (3 days ago) Jan 7
to Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and Joey Arhar
YeongHan Kim added 4 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 7:
YeongHan Kim . resolved
Thank you for your review!
The votes were reset, so I’d really appreciate it if you could take another look.
File third_party/blink/renderer/core/html/forms/html_label_element.h
want to add a TODO to remove this when the flag is removed?
YeongHan Kim
Done!
File third_party/blink/renderer/core/html/forms/html_label_element.cc
I didn't know this method exists, but want to just do `&node == this` instead?
YeongHan Kim
Done!
File third_party/blink/web_tests/external/wpt/dom/events/label-default-action.html
Line 2, Patchset 7:
<title>Label default action with interactive content</title>Joey Arhar . resolved
want to add
```
<link rel=help href="https://issues.chromium.org/issues/452084024">
```
Attention is currently required from:
- Jinho Bang
- Joey Arhar
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Jan 7, 2026, 11:53:46 AM (3 days ago) Jan 7
to YeongHan Kim, Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and YeongHan Kim
Joey Arhar voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Jinho Bang
- YeongHan Kim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
Jan 9, 2026, 2:02:01 AM (yesterday) Jan 9
to Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and Mason Freed
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Patchset-level comments
YeongHan Kim . resolved
Please take a look. Thank you!
Attention is currently required from:
- Jinho Bang
- Mason Freed
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Jan 9, 2026, 2:11:04 PM (yesterday) Jan 9
to YeongHan Kim, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang and YeongHan Kim
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
Patchset-level comments
Mason Freed . resolved
LGTM, just two changes
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 155, Patchset 8 (Latest):
if (!node || !IsShadowIncludingInclusiveAncestorOf(*node))Mason Freed . unresolved
Can you add this here, to remind us later:
`DCHECK(!RuntimeEnabledFeatures::LabelInteractiveContentCheckBeforeHandlerEnabled());`
Line 167, Patchset 8 (Latest):
DCHECK(evt.HasEventPath());Mason Freed . unresolved
...and same here without the `!`
Related details
Attention is currently required from:
- Jinho Bang
- YeongHan Kim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
Jan 9, 2026, 9:33:55 PM (19 hours ago) Jan 9
to Mason Freed, Jinho Bang, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jinho Bang, Joey Arhar and Mason Freed
YeongHan Kim added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Thank you for your review!
It looks like the votes were reset due to the DCHECK change. Sorry for the trouble, but could you please take another look?
Thank you so much!
File third_party/blink/renderer/core/html/forms/html_label_element.cc
Line 155, Patchset 8:
if (!node || !IsShadowIncludingInclusiveAncestorOf(*node))Mason Freed . resolved
Can you add this here, to remind us later:
`DCHECK(!RuntimeEnabledFeatures::LabelInteractiveContentCheckBeforeHandlerEnabled());`
YeongHan Kim
Done!
...and same here without the `!`
Attention is currently required from:
- Jinho Bang
- Joey Arhar
- Mason Freed
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages