Preserve focus when entering the back-forward cache [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@jar...@chromium.org - PTAL, do you have an idea who would be a good reviewer?
thanks in advance, and no time pressure from my end, whenver you have a couple of minutes. thanks!
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
// 5) Navigating back to |url_1|, we shouldn't restore the focus to theDo you know why we were testing for the opposite behaviour? Does the HTML spec have an opinion on this?
Related details
- Fergal Daly
- Helmut Januschka
- Joey Arhar
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. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
already seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
// 5) Navigating back to |url_1|, we shouldn't restore the focus to theDo you know why we were testing for the opposite behaviour? Does the HTML spec have an opinion on this?
The opposite expectation (`blurCount == 1`) was added in 2020 by https://crrev.com/c/2362046, intentionally clearing focus + text-input state on BFCache entry so the IME / text-input plumbing matched a normal cross-document navigation. The test was just locking in that implementation choice; not a spec requirement.
already seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Acknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Related details
- Fergal Daly
- Fergal Daly
- Joey Arhar
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. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Acknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Unfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
Related details
- Fergal Daly
- Helmut Januschka
- Joey Arhar
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Unfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
sorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Related details
- Fergal Daly
- Fergal Daly
- Joey Arhar
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. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Helmut JanuschkaUnfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
sorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Sorry if it was unclear. I think we should clarify the spec. The outcome could go either way.
Related details
- Fergal Daly
- Helmut Januschka
- Joey Arhar
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Helmut JanuschkaUnfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
Fergal Dalysorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Sorry if it was unclear. I think we should clarify the spec. The outcome could go either way.
spec issue: https://github.com/whatwg/html/issues/12496 and has a draft-pr, i'll park this CL!
Related details
- Fergal Daly
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Helmut JanuschkaUnfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
Fergal Dalysorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Helmut JanuschkaSorry if it was unclear. I think we should clarify the spec. The outcome could go either way.
spec issue: https://github.com/whatwg/html/issues/12496 and has a draft-pr, i'll park this CL!
- Fergal Daly
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Helmut JanuschkaUnfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
Fergal Dalysorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Helmut JanuschkaSorry if it was unclear. I think we should clarify the spec. The outcome could go either way.
Fergal Dalyspec issue: https://github.com/whatwg/html/issues/12496 and has a draft-pr, i'll park this CL!
Thanks. Great issue, makes things very clear.
I think the review can continue here. The fact that the note about the behavior is non-normative in the spec might just be because the spec is already in line with the note. It's just a clarification note.
Related details
- Fergal Daly
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut Januschkaalready seems to test the same thing (at least by its description). It's also failing on Firefox, so I wonder if there is consensus on the behaviour.
Fergal DalyAcknowledged, dropped `third_party/blink/web_tests/http/tests/back-forward-cache/focus-preserved.html`.
On consensus: Safari already preserves focus across BFCache, so this CL aligns Chromium with WebKit. Firefox is the other outlier then. The HTML spec is silent about it (`reactivate a Document` says nothing about focus).
I'm happy to file a spec issue to make this explicit. Want me to file it as a follow-up to this CL, or block on it?
Helmut JanuschkaUnfortunately, I think we should block. The focus behaviour was added very intentionally.
and there's a real question as to what the correct behaviour should be. There's a tension with BFCache between making it seem exactly like a normal navigation just faster and the fact that web pages are stateful and we are restoring that state.
I'm also hesistant to change focus stuff. I know we already have a mess because input events are not on the same mojo pipe as RFH and other navigation events, including focus. I feel like this has been a source of flakiness, although it was quite a while ago.
Fergal Dalysorry if that might sound dumb, am i correct (dont want to mess things up), that i should file a spec update? and then we can block on this?
or as the last part of your reply, which is totally understandable, signalizes, should we abandon this CL and close the issue with wont-fix?
Helmut JanuschkaSorry if it was unclear. I think we should clarify the spec. The outcome could go either way.
Fergal Dalyspec issue: https://github.com/whatwg/html/issues/12496 and has a draft-pr, i'll park this CL!
Rakina Zata AmniThanks. Great issue, makes things very clear.
I think the review can continue here. The fact that the note about the behavior is non-normative in the spec might just be because the spec is already in line with the note. It's just a clarification note.
- Fergal Daly
- Fergal Daly
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
If you have rakina@ already, you don't need me.
Related details
- Fergal Daly
- Helmut Januschka
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 6 comments![]( "Open in Gerrit")
Looks like the WPT is failing?
if (subframe_view != main_view) {Is it possible for this to be the same view as some other subframe's view?
// The view remains registered (its DOM focus is preserved across BFCache),
// but it is no longer the active text-input target while the page is frozen.
// Reset the cached type so that subsequent observers see NONE, mirroring what
// would happen if focus had been cleared on pagehide.
text_input_state_map_[view]->type = ui::TEXT_INPUT_TYPE_NONE;
active_view_ = nullptr;Does this need to be undone when restoring from BFCache? (see also the comment on the other file)
RemoveFocusAndTextInputState(Please add comments
widget->UpdateTextInputState();Please fix this WARNING reported by autoreview issue finding: When `clear_focus` is false (i.e. entering BFCache), `UpdateTextInputState()` will find that the text input state hasn't changed from its cached value, so it won't send an IPC. This means the renderer's cached state remains `TEXT_INPUT_TYPE_TEXT`.
However, when the page enters BFCache, the browser manually resets its state to `NONE` via `TextInputManager::DidEnterBackForwardCache`.
When the page is eventually restored from BFCache, the renderer will not send an `UpdateTextInputState` IPC (because its cached state is still `TEXT_INPUT_TYPE_TEXT`), leaving the browser permanently out of sync (stuck in `NONE`). This will break IME and virtual keyboards upon BFCache restore.
Consider explicitly clearing the widget's text input state (e.g. `widget->ClearTextInputState()`) upon entering or restoring from BFCache so that an update is forced on restore.
Related details
- Fergal Daly
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
Helmut JanuschkaPlease add some explanation
Done
Is it possible for this to be the same view as some other subframe's view?
changed this to track already-notified views, so shared subframe views are only notified once.
// The view remains registered (its DOM focus is preserved across BFCache),
// but it is no longer the active text-input target while the page is frozen.
// Reset the cached type so that subsequent observers see NONE, mirroring what
// would happen if focus had been cleared on pagehide.
text_input_state_map_[view]->type = ui::TEXT_INPUT_TYPE_NONE;
active_view_ = nullptr;Does this need to be undone when restoring from BFCache? (see also the comment on the other file)
we no longer reset the cached TextInputState type to NONE on entry, we preserve the cached state and only clear `active_view_` while the page is frozen.
On restore, the renderer re-sends its TextInputState (forced by clearing the widget's cached state on persisted pagehide), which re-establishes `active_view_` and the type in the browser.
RemoveFocusAndTextInputState(Helmut JanuschkaPlease add comments
Done
Please fix this WARNING reported by autoreview issue finding: When `clear_focus` is false (i.e. entering BFCache), `UpdateTextInputState()` will find that the text input state hasn't changed from its cached value, so it won't send an IPC. This means the renderer's cached state remains `TEXT_INPUT_TYPE_TEXT`.
However, when the page enters BFCache, the browser manually resets its state to `NONE` via `TextInputManager::DidEnterBackForwardCache`.
When the page is eventually restored from BFCache, the renderer will not send an `UpdateTextInputState` IPC (because its cached state is still `TEXT_INPUT_TYPE_TEXT`), leaving the browser permanently out of sync (stuck in `NONE`). This will break IME and virtual keyboards upon BFCache restore.
Consider explicitly clearing the widget's text input state (e.g. `widget->ClearTextInputState()`) upon entering or restoring from BFCache so that an update is forced on restore.
- Fergal Daly
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 3 comments![]( "Open in Gerrit")
render_frame_host_->set_focused_frame_tree_node_id_for_bfcache(
focused_node);Don't save per-page state in RenderFrameHost. Can you just pass this directly to StoredPage somehow, or at least save it in Page?
frame_tree_node_->frame_tree().SetFocusedFrame(Will this trigger observers or other things that might not expect that the focus is caused by BFCache restore instead of active user interaction?
widget->UpdateTextInputState();When entering the back-forward cache (`clear_focus == false`), calling `widget->UpdateTextInputState()` here is problematic. `FinishComposingText` might have modified the state (e.g., cleared the composition range), so `UpdateTextInputState` will send an asynchronous IPC to the browser with the current text input state (which has `type != NONE` since focus wasn't cleared).
Because this IPC is asynchronous, it is highly likely to arrive at the browser *after* `TextInputManager::DidEnterBackForwardCache` has already cleared `active_view_`. When the browser processes this `TextInputStateChanged` IPC with `type != NONE` for a non-active view, it will set `active_view_ = view`, making this frozen BFCached page the active IME target again and potentially stealing focus from the newly navigated page!
To fix this, avoid sending the IPC when the page is being frozen:
```cpp
widget->FinishComposingText(false /* keep_selection */);
if (clear_focus) {
widget->UpdateTextInputState();
} else {
widget->ClearTextInputState();
}
```
Related details
- Fergal Daly
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
render_frame_host_->set_focused_frame_tree_node_id_for_bfcache(
focused_node);Don't save per-page state in RenderFrameHost. Can you just pass this directly to StoredPage somehow, or at least save it in Page?
Done, moved the stash onto `PageImpl`
Will this trigger observers or other things that might not expect that the focus is caused by BFCache restore instead of active user interaction?
`SetFocusedFrame` only sends focus IPCs + `OnFrameTreeNodeFocused` + AX update; no user-gesture/activation signal, so observers see it as a normal cross-frame focus change.
When entering the back-forward cache (`clear_focus == false`), calling `widget->UpdateTextInputState()` here is problematic. `FinishComposingText` might have modified the state (e.g., cleared the composition range), so `UpdateTextInputState` will send an asynchronous IPC to the browser with the current text input state (which has `type != NONE` since focus wasn't cleared).
Because this IPC is asynchronous, it is highly likely to arrive at the browser *after* `TextInputManager::DidEnterBackForwardCache` has already cleared `active_view_`. When the browser processes this `TextInputStateChanged` IPC with `type != NONE` for a non-active view, it will set `active_view_ = view`, making this frozen BFCached page the active IME target again and potentially stealing focus from the newly navigated page!
To fix this, avoid sending the IPC when the page is being frozen:
```cpp
widget->FinishComposingText(false /* keep_selection */);
if (clear_focus) {
widget->UpdateTextInputState();
} else {
widget->ClearTextInputState();
}
```
- Fergal Daly
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 4 comments![]( "Open in Gerrit")
Let me add kenrb@ who might know more about input / focus than me here to review those areas.
EXPECT_EQ(true,Please fix this WARNING reported by autoreview issue finding: Nit: We can use `EXPECT_TRUE` here:
```cpp
EXPECT_TRUE(EvalJs(rfh_1, "document.activeElement === input").ExtractBool());
```
Or even omit `.ExtractBool()` as `EvalJsResult` is comparable to `bool`:
```cpp
EXPECT_EQ(true, EvalJs(rfh_1, "document.activeElement === input"));
```
EXPECT_EQ(true, EvalJs(rfh_subframe_a, "document.activeElement === input")Please fix this WARNING reported by autoreview issue finding: Nit: Consider using `EXPECT_TRUE` here as well (or omitting `.ExtractBool()`).
void set_focused_frame_tree_node_id_for_bfcache(FrameTreeNodeId id) {We can actually get rid of `focused_frame_tree_node_id_for_bfcache_` entirely from `PageImpl`.
**Why this works:**
The focused frame ID is only saved in `RenderFrameHostManager::CommitPending` and subsequently read in `RenderFrameHostManager::CollectPage`. Since `CollectPage` is called synchronously downstream from `CommitPending` (via `UnloadOldFrame`), we don't need to persist this state on the `PageImpl` object. We can simply pass it down the call stack as a local variable.
**What to change:**
1. Revert the changes to `page_impl.h` (remove the getters/setters and the member variable).
2. In `RenderFrameHostManager::CommitPending`, capture the focused frame ID into a local variable (e.g., `FrameTreeNodeId focused_node_id;`) instead of stashing it on the outgoing page.
3. Update the signature of `UnloadOldFrame` to accept this `focused_node_id` and pass it in from `CommitPending`.
4. Update the signature of `CollectPage` to also accept the `focused_node_id`, passing it down from `UnloadOldFrame`.
5. In `CollectPage`, use this new argument directly when calling `stored_page->set_focused_frame_tree_node_id(focused_node_id);`.
Related details
- Fergal Daly
- Helmut Januschka
- Ken Buchanan
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
Please fix this WARNING reported by autoreview issue finding: Nit: We can use `EXPECT_TRUE` here:
```cpp
EXPECT_TRUE(EvalJs(rfh_1, "document.activeElement === input").ExtractBool());
```
Or even omit `.ExtractBool()` as `EvalJsResult` is comparable to `bool`:
```cpp
EXPECT_EQ(true, EvalJs(rfh_1, "document.activeElement === input"));
```
Done
EXPECT_EQ(true, EvalJs(rfh_subframe_a, "document.activeElement === input")Please fix this WARNING reported by autoreview issue finding: Nit: Consider using `EXPECT_TRUE` here as well (or omitting `.ExtractBool()`).
Done
void set_focused_frame_tree_node_id_for_bfcache(FrameTreeNodeId id) {We can actually get rid of `focused_frame_tree_node_id_for_bfcache_` entirely from `PageImpl`.
**Why this works:**
The focused frame ID is only saved in `RenderFrameHostManager::CommitPending` and subsequently read in `RenderFrameHostManager::CollectPage`. Since `CollectPage` is called synchronously downstream from `CommitPending` (via `UnloadOldFrame`), we don't need to persist this state on the `PageImpl` object. We can simply pass it down the call stack as a local variable.**What to change:**
1. Revert the changes to `page_impl.h` (remove the getters/setters and the member variable).
2. In `RenderFrameHostManager::CommitPending`, capture the focused frame ID into a local variable (e.g., `FrameTreeNodeId focused_node_id;`) instead of stashing it on the outgoing page.
3. Update the signature of `UnloadOldFrame` to accept this `focused_node_id` and pass it in from `CommitPending`.
4. Update the signature of `CollectPage` to also accept the `focused_node_id`, passing it down from `UnloadOldFrame`.
5. In `CollectPage`, use this new argument directly when calling `stored_page->set_focused_frame_tree_node_id(focused_node_id);`.
- Fergal Daly
- Ken Buchanan
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Buchanan (Gerrit)
Ken Buchanan added 4 comments![]( "Open in Gerrit")
#include "content/public/browser/frame_tree_node_id.h"No longer needed.
for (FrameTreeNode* node : FrameTree::SubtreeAndInnerTreeNodes(Rather than doing a manual traversal, can you just get the active widget from the TextInputManager and use `std::ranges::find_if` to see if it corresponds to one in this NodeRange?
if (focus_render_view && is_main_frame) {If the current page is not focused, why do we not want to remember which frame has focus within the current page? I think we are still preserving element-level focus within the frame.
if (focus_render_view && pending_stored_page &&Same question here. Does this lose the focused subframe if the current tab is not focused and in the foreground?
Related details
- Fergal Daly
- Helmut Januschka
- Rakina Zata Amni
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
@ke...@chromium.org thank you for the feedback, addressed all comments! let me know if you'd want me to address anything else
#include "content/public/browser/frame_tree_node_id.h"Helmut JanuschkaNo longer needed.
Done
for (FrameTreeNode* node : FrameTree::SubtreeAndInnerTreeNodes(Rather than doing a manual traversal, can you just get the active widget from the TextInputManager and use `std::ranges::find_if` to see if it corresponds to one in this NodeRange?
works! thx!
If the current page is not focused, why do we not want to remember which frame has focus within the current page? I think we are still preserving element-level focus within the frame.
Good point. Dropped the `focus_render_view` part of the guard so we always remember the focused frame within the page when swapping out the main frame. Updated the comment to note this.
Same question here. Does this lose the focused subframe if the current tab is not focused and in the foreground?
Yeah, it would have. Dropped `focus_render_view` here too.
Related details
- Fergal Daly
- Ken Buchanan
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
lgtm but rakina@ should also approve it as a full content owner
Related details
- Fergal Daly
- Helmut Januschka
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Fergal Daly
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |