Make input work for nested SurfaceEmbed [chromium/src : main]
Liang Zhao (Gerrit)
Message from Liang Zhao![]( "Open in Gerrit")
Set Ready For Review
Related details
- Dave Risney
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
@kere...@chromium.org, could you also take a look at this change before I add owners for review?
struct MultiLevelTestContext {Liang Zhaonit: In the other test file you used different casing 'Multilevel' vs 'MultiLevel' here.
Done
SurfaceEmbedConnector::Detach(child);Liang ZhaoNit: This relies on Detach calling back synchronously to have itself removed from the collection or else we end up in infinite loop. Its harder to read than more explicit code that always removes the back entry and then calls Detach if the weak ptr resolves and then the erase_if callback becomes a no-op.
swapped to a local vector before performing detaching.
for (const auto& child_weak : surface_embed_children_) {Liang ZhaoSince we're calling into other methods during iteration - any concern that we may modify the collection we're iterating over during iteration? (same for the unregister loop below)
Should be no concern. Added comments.
Related details
- Dave Risney
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Is this code to give page focus to the parent page when `RequestFocus()` is called?
Related details
- Dave Risney
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Is this code to give page focus to the parent page when `RequestFocus()` is called?
This makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
Related details
- Dave Risney
- Keren Zhu
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. |
Dave Risney (Gerrit)
Dave Risney voted Code-Review+1![]( "Open in Gerrit")
- Keren Zhu
- Liang Zhao
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. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Liang ZhaoIs this code to give page focus to the parent page when `RequestFocus()` is called?
This makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
`WebElement::FocusFromBrowser` looks a bit scary because it is not very clear how it should differ from `Focus`.
Looks like `WebElement::Focus()` eventually calls `RenderFrameHostImpl::DidFocusFrame()`. Since this originates from `RequestFocus` in the browser process, can we suppress the handling of `DidFocusFrame` when there is a pending `RequestFocus`?
Related details
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Liang ZhaoIs this code to give page focus to the parent page when `RequestFocus()` is called?
Keren ZhuThis makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
`WebElement::FocusFromBrowser` looks a bit scary because it is not very clear how it should differ from `Focus`.
Looks like `WebElement::Focus()` eventually calls `RenderFrameHostImpl::DidFocusFrame()`. Since this originates from `RequestFocus` in the browser process, can we suppress the handling of `DidFocusFrame` when there is a pending `RequestFocus`?
The order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
This is similar to Frame::FocusImpl code used by LocalFrameMojoHandler::Focus() and RemoteFrame::Focus() methods for iframes that handles cross process focused frame changes.
Not sure whether there is a better option.
Related details
- Keren Zhu
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. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Liang ZhaoIs this code to give page focus to the parent page when `RequestFocus()` is called?
Keren ZhuThis makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
Liang Zhao`WebElement::FocusFromBrowser` looks a bit scary because it is not very clear how it should differ from `Focus`.
Looks like `WebElement::Focus()` eventually calls `RenderFrameHostImpl::DidFocusFrame()`. Since this originates from `RequestFocus` in the browser process, can we suppress the handling of `DidFocusFrame` when there is a pending `RequestFocus`?
The order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
This is similar to Frame::FocusImpl code used by LocalFrameMojoHandler::Focus() and RemoteFrame::Focus() methods for iframes that handles cross process focused frame changes.
Not sure whether there is a better option.
The order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
`DidFocusFrame` is a result of `RequestFocus`. Even if they are on associated mojo interfaces, we must receive `DidFocusFrame` after `RequestFocus`?
Related details
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Liang ZhaoIs this code to give page focus to the parent page when `RequestFocus()` is called?
Keren ZhuThis makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
Liang Zhao`WebElement::FocusFromBrowser` looks a bit scary because it is not very clear how it should differ from `Focus`.
Looks like `WebElement::Focus()` eventually calls `RenderFrameHostImpl::DidFocusFrame()`. Since this originates from `RequestFocus` in the browser process, can we suppress the handling of `DidFocusFrame` when there is a pending `RequestFocus`?
Keren ZhuThe order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
This is similar to Frame::FocusImpl code used by LocalFrameMojoHandler::Focus() and RemoteFrame::Focus() methods for iframes that handles cross process focused frame changes.
Not sure whether there is a better option.
The order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
`DidFocusFrame` is a result of `RequestFocus`. Even if they are on associated mojo interfaces, we must receive `DidFocusFrame` after `RequestFocus`?
Method calls on associated mojo interfaces are guaranteed to be received in the other end in the same order they are called. So, if they are on associated mojo interfaces, we can wait for RequestFocus to complete before we call SetAsFocusedWebContentsIfNecessary() for WebContentsImpl::FocusOwningWebContents(), and then we could have a solution. The DidFocusFrame happens before RequestFocus callback is invoked and therefore it will be received before we do SetAsFocusedWebContentsIfNecessary().
But we might have a solution without this new FocusFromBrowser(). If we make focused frame to be set in browser process before do RequestFocus(), then we will ignore the DidFocusFrame() when we process it in FrameTree::SetFocusedFrame(). So if we do ReqiestFocus() after SetAsFocusedWebContentsIfNecessary() in WebContentsImpl::FocusOwningWebContents(), it might work. if SetAsFocusedWebContentsIfNecessary() doesn't actually do SetFocusedFrame() in parent document, we can simply call RenderFrameHostImpl::SetFocusedFrame() before we do surface_embed_->RequestFocus().
Let me try it out.
Related details
- Keren Zhu
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Liang ZhaoIs this code to give page focus to the parent page when `RequestFocus()` is called?
Keren ZhuThis makes the frame focused frame so that when element->Focus() is called, it does not trigger focused frame change event (FocusController::SetFocusedElement() calls FocusController::SetFocusedFrame which will notify browser process if focused frame changed unless notify_embedder is false. This call is to ensure that focused frame change happens with notify_embedder as false. Let me add some comments.
The page focus update is still done by browser process and more involved.
Liang Zhao`WebElement::FocusFromBrowser` looks a bit scary because it is not very clear how it should differ from `Focus`.
Looks like `WebElement::Focus()` eventually calls `RenderFrameHostImpl::DidFocusFrame()`. Since this originates from `RequestFocus` in the browser process, can we suppress the handling of `DidFocusFrame` when there is a pending `RequestFocus`?
Keren ZhuThe order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
This is similar to Frame::FocusImpl code used by LocalFrameMojoHandler::Focus() and RemoteFrame::Focus() methods for iframes that handles cross process focused frame changes.
Not sure whether there is a better option.
Liang ZhaoThe order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
`DidFocusFrame` is a result of `RequestFocus`. Even if they are on associated mojo interfaces, we must receive `DidFocusFrame` after `RequestFocus`?
Method calls on associated mojo interfaces are guaranteed to be received in the other end in the same order they are called. So, if they are on associated mojo interfaces, we can wait for RequestFocus to complete before we call SetAsFocusedWebContentsIfNecessary() for WebContentsImpl::FocusOwningWebContents(), and then we could have a solution. The DidFocusFrame happens before RequestFocus callback is invoked and therefore it will be received before we do SetAsFocusedWebContentsIfNecessary().
But we might have a solution without this new FocusFromBrowser(). If we make focused frame to be set in browser process before do RequestFocus(), then we will ignore the DidFocusFrame() when we process it in FrameTree::SetFocusedFrame(). So if we do ReqiestFocus() after SetAsFocusedWebContentsIfNecessary() in WebContentsImpl::FocusOwningWebContents(), it might work. if SetAsFocusedWebContentsIfNecessary() doesn't actually do SetFocusedFrame() in parent document, we can simply call RenderFrameHostImpl::SetFocusedFrame() before we do surface_embed_->RequestFocus().
Let me try it out.
Trying to make browser side update its focused frame doesn't seem to work.
There is no check on whether focused frame changed in RenderFrameHostImpl::DidFocusFrame -> FrameTreeNode::SetFocusedFrame -> WebContentsImpl::SetFocusedFrame -> WebContentsImpl::SetFocusedFrameTree code path. Updating these functions seems to have more risk.
Also, calling FrameTreeNode::SetFocusedFrame() or WebContentsImpl::SetFocusedFrame() would update focused frame tree which puts us in an incorrect temporarily.
So, the only way to avoid the FocusFromBrowser() would be to make SurfaceEmbed and Frame mojo interfaces associated. A little more work, but it is possible to make the mojo interfaces associated. If we make them associated, we can simply do auto* node = embedder_rfh_->frame_tree_node() and then node->frame_tree().SetFocusedFrame(node, nullptr) in SurfaceEmbedConnectorImpl::SetFocusedFrameTree, conceptually as part of FocusOuterFrameTrees. Using associated mojo interfaces could potentially make other things more deterministic.
@kere...@chromium.org, do you think that we should use frame associated interface? If you think that is better, I can create another CL to make the change.
Related details
- Dave Risney
- Keren Zhu
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. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
I agreed with you that setting focused frame in the browser process to suppress `RenderFrameHostImpl::DidFocusFrame` is too risky and it leaves the browser in a temporary wrong state.
Yes I think we should use associated interface. This has been a problem since day one which I haven't had time to look into. Now I think it is time, given that messages in surface_embed.mojom start to intertwine with messages on associated interfaces.
The order of DidFocusFrame and RequestFocus callback happens on un-associated mojo interface and the order could not be guaranteed.
Looking back at your previous comment, I think I missed "callback" in this comment. I was arguing that the "RequestFocus" call must happen before the "DidFocusFrame" call, but what actually matters is the firing order between "RequestFocus callback" and "DidFocusFrame", which has no guarantee. Sorry I missed that.
And as always, thanks for looking into this.
Related details
- Dave Risney
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Created CL that moves to associated interfaces: https://chromium-review.googlesource.com/c/chromium/src/+/8142467. Will move this CL based on that one and see whether focus could work better without this FocusFromBrowser. The plan is to ensure embedder_rfh is the focused frame when we are doing FocusOuterFrameTrees, and move RequestFocus to be after SetAsFocusedWebContentsIfNecessary in FocusOwningWebContents.
Related details
- Dave Risney
- Keren Zhu
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (frame && frame->GetPage()) {
frame->GetPage()->GetFocusController().FocusDocumentView(
frame, /*notify_embedder=*/false);
}Updated and no longer need this FocusFromBrowser().
Related details
- Dave Risney
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao removed performance-m...@chromium.org from this change![]( "Open in Gerrit")
Related details
- Dave Risney
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao removed blink-...@chromium.org from this change![]( "Open in Gerrit")
Liang Zhao (Gerrit)
Liang Zhao removed blink-re...@chromium.org from this change![]( "Open in Gerrit")
Keren Zhu (Gerrit)
Keren Zhu added 7 comments![]( "Open in Gerrit")
This looks great. I don't understand some parts so leave some questions.
- SurfaceEmbedHost::OnEmbedElementFocused didn't focus on child
webcontents when handling Tab traversal and relied on the wrong
logic in WebContentsImpl::SetFocusedFrame to pass the test. AddedWhat's the wrong logic? My understanding is that `<embeb>.focus()` -> `SurfaceEmbedHost::OnEmbedElementFocused` -> `child_contents_->FocusThroughTabTraversal`, which will focus the child WebContents?
webcontents for kPageAction event and could potentially set focusedDo you mean `FocusType::kPage`?
element. To fix this, we set focus on the owning frame as part of
updating outer frame tree focus when setting focused frame tree andCan you point me to the code that does this? Is it https://chromium-review.git.corp.google.com/c/chromium/src/+/8102721/15/content/browser/web_contents/web_contents_impl.cc#10771?
if (!focused) {
// TODO(crbug.com/508638062): the <embed> element in the parent frame lost
// focus. Should we remove the page focus from the embedded page?
return;
}For my own understanding, I think this test case will fail,
```
<html>
<embed id="parent">
<embed id="child"> ... </embed>
<input>
</embed>
</html>
```
Assuming the focus starts within #child then JS calls `<input>.focus()`. Expected: #parent's WebContents gets focus; Actual: #parent's WebContents is not focused and #child's WebContents remains to be focused.
Looks like this needs to be fixed by implementing the blur logic around here (not for this CL).
if (!child_contents_->ContainsOrIsFocusedWebContents()) {
child_contents_->Focus();
}I think this is only needed because `SurfaceEmbedConnectorImpl::RequestFocus()` walks up the <embed> list. What if we walk down the list from the root <embed>?
void SurfaceEmbedConnectorImpl::RequestFocus() {To distinguish between focusing WebContents from focusing <embed>, maybe rename `RequestFocus()` -> `RequestFocusOnEmbedElement()`, for both `SurfaceEmbedConnectorImpl` and `SurfaceEmbedHost`.
Related details
- Dave Risney
- Liang Zhao
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. |