Fix focus emulation not persisting on navigations [chromium/src : main]
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
PTAL! It looks like there are at least two normal paths to restore focus state but none actually works with the focused page emulation. WDYT about this solution?
Related details
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dmitry Gozman
| Code-Review | +1 |
1 comment
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
masonf@ PTAL at another approach to fixing the DevTools focused page emulation.
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
Seems reasonable to me! Good luck with this attempt.
SetFocusedFrame(page_->MainFrame());nit: maybe just `frame` here?
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
nit: maybe just `frame` here?
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
It seems there is still a situation where this change (seems to be on reload with iframes on the page?) causes a DCHECK to be hit:
```
[3253951:1:0710/134552.699054:FATAL:third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758] DCHECK failed: context.IsEmpty() || frame == ToLocalFrameIfNotDetached(context).
#0 0x7f4c0a9265e2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1052:7]
#1 0x7f4c0a9027e1 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:255:20]
#2 0x7f4c0a7c2a1a logging::LogMessage::Flush() [../../base/logging.cc:705:29]
#3 0x7f4c0a7c28f0 logging::LogMessage::~LogMessage() [../../base/logging.cc:694:3]
#4 0x7f4c0a7a0981 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() [../../base/check.cc:147:3]
#5 0x7f4c0a7a0163 logging::CheckError::~CheckError() [../../third_party/libc++/src/include/__memory/unique_ptr.h:77:5]
#6 0x7f4bfaf3ef97 blink::ToV8ContextMaybeEmpty() [../../third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758:3]
#7 0x7f4bfd2112a4 blink::Document::UpdateStyleAndLayoutTreeForThisDocument() [../../third_party/blink/renderer/core/dom/document.cc:2539:3]
#8 0x7f4bfd210e14 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2464:5]
#9 0x7f4bfd20ae34 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2439:3]
#10 0x7f4bfb83c4e0 blink::FrameSelection::FocusedOrActiveStateChanged() [../../third_party/blink/renderer/core/editing/frame_selection.cc:1044:17]
#11 0x7f4bfc919228 blink::FocusController::SetFocusedFrame() [../../third_party/blink/renderer/core/page/focus_controller.cc:1364:28]
#12 0x7f4bfc91c727 blink::FocusController::UpdateFocusOnNavigationCommit() [../../third_party/blink/renderer/core/page/focus_controller.cc:1533:5]
#13 0x7f4bfc77d741 blink::DocumentLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/document_loader.cc:2987:43]
#14 0x7f4bfc7bbdfe blink::FrameLoader::CommitDocumentLoader() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1417:21]
#15 0x7f4bfc7cc8ff blink::FrameLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1240:3]
#16 0x7f4bfbc790ae blink::WebLocalFrameImpl::CommitNavigation() [../../third_party/blink/renderer/core/frame/web_local_frame_impl.cc:2791:24]
#17 0x7f4c072961ef content::RenderFrameImpl::CommitNavigationWithParams() [../../content/renderer/render_frame_impl.cc:2930:11]
```
Apparently, setting a focused frame on navigation commit might be too early.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
It seems there is still a situation where this change (seems to be on reload with iframes on the page?) causes a DCHECK to be hit:
```
[3253951:1:0710/134552.699054:FATAL:third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758] DCHECK failed: context.IsEmpty() || frame == ToLocalFrameIfNotDetached(context).
#0 0x7f4c0a9265e2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1052:7]
#1 0x7f4c0a9027e1 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:255:20]
#2 0x7f4c0a7c2a1a logging::LogMessage::Flush() [../../base/logging.cc:705:29]
#3 0x7f4c0a7c28f0 logging::LogMessage::~LogMessage() [../../base/logging.cc:694:3]
#4 0x7f4c0a7a0981 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() [../../base/check.cc:147:3]
#5 0x7f4c0a7a0163 logging::CheckError::~CheckError() [../../third_party/libc++/src/include/__memory/unique_ptr.h:77:5]
#6 0x7f4bfaf3ef97 blink::ToV8ContextMaybeEmpty() [../../third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758:3]
#7 0x7f4bfd2112a4 blink::Document::UpdateStyleAndLayoutTreeForThisDocument() [../../third_party/blink/renderer/core/dom/document.cc:2539:3]
#8 0x7f4bfd210e14 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2464:5]
#9 0x7f4bfd20ae34 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2439:3]
#10 0x7f4bfb83c4e0 blink::FrameSelection::FocusedOrActiveStateChanged() [../../third_party/blink/renderer/core/editing/frame_selection.cc:1044:17]
#11 0x7f4bfc919228 blink::FocusController::SetFocusedFrame() [../../third_party/blink/renderer/core/page/focus_controller.cc:1364:28]
#12 0x7f4bfc91c727 blink::FocusController::UpdateFocusOnNavigationCommit() [../../third_party/blink/renderer/core/page/focus_controller.cc:1533:5]
#13 0x7f4bfc77d741 blink::DocumentLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/document_loader.cc:2987:43]
#14 0x7f4bfc7bbdfe blink::FrameLoader::CommitDocumentLoader() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1417:21]
#15 0x7f4bfc7cc8ff blink::FrameLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1240:3]
#16 0x7f4bfbc790ae blink::WebLocalFrameImpl::CommitNavigation() [../../third_party/blink/renderer/core/frame/web_local_frame_impl.cc:2791:24]
#17 0x7f4c072961ef content::RenderFrameImpl::CommitNavigationWithParams() [../../content/renderer/render_frame_impl.cc:2930:11]
```Apparently, setting a focused frame on navigation commit might be too early.
Interestingly, @pfa...@chromium.org and I were not able to reproduce this issue again. If we are still unsuccessful, we plant to land this after the branch point.
P.S. there seem to be other bugs that might cause this DCHECK to be hit crbug.com/397385670
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Fixed: 40285513Related details
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Alex RudenkoIt seems there is still a situation where this change (seems to be on reload with iframes on the page?) causes a DCHECK to be hit:
```
[3253951:1:0710/134552.699054:FATAL:third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758] DCHECK failed: context.IsEmpty() || frame == ToLocalFrameIfNotDetached(context).
#0 0x7f4c0a9265e2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1052:7]
#1 0x7f4c0a9027e1 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:255:20]
#2 0x7f4c0a7c2a1a logging::LogMessage::Flush() [../../base/logging.cc:705:29]
#3 0x7f4c0a7c28f0 logging::LogMessage::~LogMessage() [../../base/logging.cc:694:3]
#4 0x7f4c0a7a0981 logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() [../../base/check.cc:147:3]
#5 0x7f4c0a7a0163 logging::CheckError::~CheckError() [../../third_party/libc++/src/include/__memory/unique_ptr.h:77:5]
#6 0x7f4bfaf3ef97 blink::ToV8ContextMaybeEmpty() [../../third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.cc:758:3]
#7 0x7f4bfd2112a4 blink::Document::UpdateStyleAndLayoutTreeForThisDocument() [../../third_party/blink/renderer/core/dom/document.cc:2539:3]
#8 0x7f4bfd210e14 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2464:5]
#9 0x7f4bfd20ae34 blink::Document::UpdateStyleAndLayoutTree() [../../third_party/blink/renderer/core/dom/document.cc:2439:3]
#10 0x7f4bfb83c4e0 blink::FrameSelection::FocusedOrActiveStateChanged() [../../third_party/blink/renderer/core/editing/frame_selection.cc:1044:17]
#11 0x7f4bfc919228 blink::FocusController::SetFocusedFrame() [../../third_party/blink/renderer/core/page/focus_controller.cc:1364:28]
#12 0x7f4bfc91c727 blink::FocusController::UpdateFocusOnNavigationCommit() [../../third_party/blink/renderer/core/page/focus_controller.cc:1533:5]
#13 0x7f4bfc77d741 blink::DocumentLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/document_loader.cc:2987:43]
#14 0x7f4bfc7bbdfe blink::FrameLoader::CommitDocumentLoader() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1417:21]
#15 0x7f4bfc7cc8ff blink::FrameLoader::CommitNavigation() [../../third_party/blink/renderer/core/loader/frame_loader.cc:1240:3]
#16 0x7f4bfbc790ae blink::WebLocalFrameImpl::CommitNavigation() [../../third_party/blink/renderer/core/frame/web_local_frame_impl.cc:2791:24]
#17 0x7f4c072961ef content::RenderFrameImpl::CommitNavigationWithParams() [../../content/renderer/render_frame_impl.cc:2930:11]
```Apparently, setting a focused frame on navigation commit might be too early.
Interestingly, @pfa...@chromium.org and I were not able to reproduce this issue again. If we are still unsuccessful, we plant to land this after the branch point.
P.S. there seem to be other bugs that might cause this DCHECK to be hit crbug.com/397385670
We found a repro: reloading a page restored from bfcache causes the DCHECK failure. So far I am not sure why?
Related details
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
So I could not find the reason for bfcache restored pages behaving differently but I moved the DCHECK logic to the document_loader and saw that it can be hit in other situations as well. It appears that the safe place to restore the focus would be after the document response starts loading. At that point, the frame is already not counted as detached. I do not see any immediate drawbacks of moving the logic (I will see what tests say) but I wonder what others think? I also added a bfcache specific test.
Related details
- Dmitry Gozman
- Fergal Daly
- Mason Freed
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Will wait for green bots, if that's ok.
Related details
- Alex Rudenko
- Dmitry Gozman
- Fergal Daly
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
It seems there is still a situation where this change (seems to be on reload with iframes on the page?) causes a DCHECK to be hit:
So I waited long enough and it seems someone fixed the DCHECK I was encountering.
Related details
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
Changes since patchset 7 look good. Just one comment.
bool was_focused_frame = old_document_info_for_commit &&You don't need this, given the `if()`. Perhaps you don't even need this `was_focused_frame` local var, given that it's only used one place.
Related details
- Alex Rudenko
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
bool was_focused_frame = old_document_info_for_commit &&You don't need this, given the `if()`. Perhaps you don't even need this `was_focused_frame` local var, given that it's only used one place.
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
27 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/loader/document_loader.cc
Insertions: 1, Deletions: 3.
@@ -2989,10 +2989,8 @@
}
if (old_document_info_for_commit) {
- bool was_focused_frame = old_document_info_for_commit &&
- old_document_info_for_commit->was_focused_frame;
frame_->GetPage()->GetFocusController().UpdateFocusOnNavigationCommit(
- frame_, was_focused_frame);
+ frame_, old_document_info_for_commit->was_focused_frame);
if (old_document_info_for_commit->overlay_color.has_value()) {
frame_->SetFrameColorOverlay(
old_document_info_for_commit->overlay_color.value());
```
Change information
Fix focus emulation not persisting on navigations
This is a reland of crrev.com/c/6618646 with ideas from
crrev.com/c/6633595.
Currently, the focused page emulation is broken for navigations and
reloads that rely on the agent state to be restored. The agent state is
restored too early and the focused frame gets removed by Blink.
Normally, the focus is restored after a navigation via the browser
process [1] or by blink itself [2]. The scenario [1] does not work
because the restoration depends on the browser state about the focused
view whereas the emulation happens solely in blink. The scenario [2]
seems to depend on feature flags and it is not taken in the default
configuration due to the focused frame being reset in [3] making the
logic in the loader think the frame was not focused.
This CL fixes the original issue by making sure the FocusController
handles navigation commits taking into account the navigation commit.
This CL does not fix the issue that is possible to undo the emulation
using the Tab key and this CL does not affect the ability of focus to
different frames.
[1]:
https://crsrc.org/c/content/browser/renderer_host/render_frame_host_manager.cc;l=5207;drc=f70a0969b8b5dea5d99de43de959479d7f9cf603
[2]:
https://crsrc.org/c/third_party/blink/renderer/core/loader/document_loader.cc;l=2988;drc=de55cb27badd6107c790a271520f41e9228ad5ab
[3]:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/exported/web_view_impl.cc;l=2692;drc=36ddf4a08efa862b60953b7d3ef82c8ce6c99914
- M third_party/blink/renderer/core/inspector/inspector_emulation_agent.cc
- M third_party/blink/renderer/core/loader/document_loader.cc
- M third_party/blink/renderer/core/page/focus_controller.cc
- M third_party/blink/renderer/core/page/focus_controller.h
- M third_party/blink/renderer/core/page/focus_controller_test.cc
- A third_party/blink/web_tests/http/tests/inspector-protocol/bfcache/bfcache-focus-emulation-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/bfcache/bfcache-focus-emulation.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/emulation/resources/focus-on-load.html
- A third_party/blink/web_tests/http/tests/inspector-protocol/emulation/set-focus-emulation-enabled-persists-navigations-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/emulation/set-focus-emulation-enabled-persists-navigations.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/emulation/set-focus-emulation-onload-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/emulation/set-focus-emulation-onload.js
Code-Review: +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |