Fix stale :focus-within after synchronous focus shifts [chromium/src : main]
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Related details
- Mason Freed
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
I'm not too familiar with how this is supposed to work, in which order, but I do remember having looked into a similar issue which may, or may not, be relevant to think about here:
https://issues.chromium.org/u/1/issues/439339105#comment8
I'll leave it to Mason to review. I can give a second +1 when necessary.
Related details
- Mason Freed
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
- Mason Freed
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
I'm not too familiar with how this is supposed to work, in which order, but I do remember having looked into a similar issue which may, or may not, be relevant to think about here:
https://issues.chromium.org/u/1/issues/439339105#comment8
I'll leave it to Mason to review. I can give a second +1 when necessary.
- Mason Freed
- Rune Lillesveen
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)
Mason Freed added 5 comments![]( "Open in Gerrit")
Thanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
if (ancestor && !ancestor->contains(focused_element_)) {This is a parent tree walk (O(tree depth)). This one will get hit every time the focus changes and there's a common ancestor, which is quite common.
ancestor->SetHasFocusWithinUpToAncestor(This is also a parent tree walk (O(distance to common ancestor)).
DynamicTo<Element>(FlatTreeTraversal::CommonAncestor(This is also several parent tree walks (O(distance to common ancestor)).
if (ancestor && !ancestor->contains(focused_element_)) {
ancestor->SetHasFocusWithinUpToAncestor(
false,
DynamicTo<Element>(FlatTreeTraversal::CommonAncestor(
*ancestor, *focused_element_)),
/*need_snap_container_search=*/false);
}ditto here
Related details
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Thanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
Hi, thanks for the feedback!
I tried to use Pinpoint as you suggested, but unfortunately, I was denied access. Could you help me with running the regression test?
For the benchmarks, I believe blink_perf.css and blink_perf.events are appropriate. Do I need to run anything else besides these?
Related details
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Suyeon JiThanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
Hi, thanks for the feedback!
I tried to use Pinpoint as you suggested, but unfortunately, I was denied access. Could you help me with running the regression test?
For the benchmarks, I believe blink_perf.css and blink_perf.events are appropriate. Do I need to run anything else besides these?
Sorry for the delay here! I just kicked off these pinpoints:
Related details
- Suyeon Ji
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16bf8e53710000
Related details
- Suyeon Ji
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-perf/blink_perf.events failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17809826710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-perf/blink_perf.css failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12630e4e710000
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Suyeon JiThanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
Mason FreedHi, thanks for the feedback!
I tried to use Pinpoint as you suggested, but unfortunately, I was denied access. Could you help me with running the regression test?
For the benchmarks, I believe blink_perf.css and blink_perf.events are appropriate. Do I need to run anything else besides these?
Suyeon JiSorry for the delay here! I just kicked off these pinpoints:
Hi, thanks for the review. I'm sorry, but the perf test failed.
It seems that both my test and the baseline failed. Since I can't see the logs, could you please take a look?
Also, I share your concern about the tree walks, so I'll give it some more thought.
Related details
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Suyeon JiThanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
Mason FreedHi, thanks for the feedback!
I tried to use Pinpoint as you suggested, but unfortunately, I was denied access. Could you help me with running the regression test?
For the benchmarks, I believe blink_perf.css and blink_perf.events are appropriate. Do I need to run anything else besides these?
Suyeon JiSorry for the delay here! I just kicked off these pinpoints:
Hi, thanks for the review. I'm sorry, but the perf test failed.
It seems that both my test and the baseline failed. Since I can't see the logs, could you please take a look?
Also, I share your concern about the tree walks, so I'll give it some more thought.
YEah, sorry about that, I think Pinpoint had some issues over the last few days. I believe they're fixed, so I just kicked off these two:
Related details
- Suyeon Ji
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.events complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10e0ed17710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.events complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14bb4c6e710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.events complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/107221d6710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.events complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13f4204a710000
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Suyeon JiThanks for the fix! The code looks correct, but I'm a little worried about performance regressions for changing focus. Are you familiar with the pinpoint system for checking for regressions like this? If not, I can help kick off a test.
Mason FreedHi, thanks for the feedback!
I tried to use Pinpoint as you suggested, but unfortunately, I was denied access. Could you help me with running the regression test?
For the benchmarks, I believe blink_perf.css and blink_perf.events are appropriate. Do I need to run anything else besides these?
Suyeon JiSorry for the delay here! I just kicked off these pinpoints:
Mason FreedHi, thanks for the review. I'm sorry, but the perf test failed.
It seems that both my test and the baseline failed. Since I can't see the logs, could you please take a look?
Also, I share your concern about the tree walks, so I'll give it some more thought.
YEah, sorry about that, I think Pinpoint had some issues over the last few days. I believe they're fixed, so I just kicked off these two:
Checking the test results, there doesn't seem to be any regression.
IMHO, this will affect performance much.
Usually, `focused_element_` is initialized to `nullptr` inside `SetFocusedElement()` ([Line 5894](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=5894)). In this case, the new code is skipped because it only runs when `focused_element_` is non-null.
`focused_element_` is non-null only when `SetFocusedElement()` is called recursively, like calling focus() while dispatching blur or focusout events.
Since this rarely happens, the performance impact is likely small.
Please let me know what you think. Thank you!
Related details
- Mason Freed
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. |
Suyeon Ji (Gerrit)
*this will not affect performance much.
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
Ok, let's land it and see what happens. Thanks for the patch!
Related details
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 6 comments![]( "Open in Gerrit")
Ok, let's land it and see what happens. Thanks for the patch!
Thank you!
if (ancestor && !ancestor->contains(focused_element_)) {This is a parent tree walk (O(tree depth)). This one will get hit every time the focus changes and there's a common ancestor, which is quite common.
Done
This is also a parent tree walk (O(distance to common ancestor)).
Done
This is also several parent tree walks (O(distance to common ancestor)).
Done
if (ancestor && !ancestor->contains(focused_element_)) {
ancestor->SetHasFocusWithinUpToAncestor(
false,
DynamicTo<Element>(FlatTreeTraversal::CommonAncestor(
*ancestor, *focused_element_)),
/*need_snap_container_search=*/false);
}Suyeon Jiditto here
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
rslgtm
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix stale :focus-within after synchronous focus shifts
This patch ensures :focus-within is updated correctly when focus changes
during focus/blur event dispatch.
Previously, the common ancestor used to update :focus-within was
computed before dispatching events. If a handler synchronously moved
focus, that ancestor could become invalid, leaving elements incorrectly
matching :focus-within.
This change detects focus shifts after event dispatch and re-evaluates
the ancestry, clearing the stale :focus-within state up to the correct
common ancestor when needed.
- M third_party/blink/renderer/core/dom/document.cc
- D third_party/blink/web_tests/external/wpt/css/selectors/focus-within-focus-move-expected.txt
Code-Review: +1 by Rune Lillesveen, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-perf/blink_perf.css failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/145b4963710000
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |