Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474323002:
Implement WebContentsViewChildFrame::TakeFocus. (Closed)
DescriptionImplement WebContentsViewChildFrame::TakeFocus.
TakeFocus will take focus from the child and proceed to sequentially
move focus in the parent WebContents.
+ Implement TakeFocus()
~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from
RenderFrameProxyHost to RenderFrameHostImpl.
~ Prevent blink's FocusController from focusing the main frame if no
frame is focused on a blur event.
~ Reenable 8 interactive browsertests for webview when using OOPIF.
~ Fix flakiness in OOPIF-webview versions of
WebViewFocusInteractiveTest.FocusAndVisibility and
WebViewFocusInteractiveTest.Focus_AdvanceFocus.
BUG=610819
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2474323002
Cr-Commit-Position: refs/heads/master@{#445221}
Committed: https://chromium.googlesource.com/chromium/src/+/c2ff1efb3496708d94fb12f79b3aeac6b1846b4b
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rerebase #Patch Set 4 : Conflicted with my own cl https://codereview.chromium.org/2455133005/ #
Total comments: 20
Patch Set 5 : Fix cases where focus does not actually advance focus #
Total comments: 3
Patch Set 6 : Address comments and re-enable tests #Patch Set 7 : Disable Focus_FocusBeforeNavigation until fixed. #Patch Set 8 : rebase #Patch Set 9 : Fix flaky tests and add tracking output for test run. #Patch Set 10 : Remove logging code. #
Total comments: 6
Patch Set 11 : Fix header comment as requested by dcheng. #Patch Set 12 : Rebase #Patch Set 13 : remove logging and move pant kickcing out of loop #
Messages
Total messages: 83 (65 generated)
Description was changed from ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. BUG=610819 ========== to ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avallee@chromium.org changed reviewers: + alexmos@chromium.org
Allows focus navigation with the tab key to work properly. This is in preparation to enabling some webview tests with oopif.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Thanks, this seems reasonable. A few comments below, and please also add a test for this. See SitePerProcessInteractiveBrowserTest.SequentialFocusNavigation for an example of how I did this for OOPIFs. https://codereview.chromium.org/2474323002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:363: type, (source_rfh Let's keep source_proxy defined as a separate var and pass it here. I think it makes the code a bit easier to follow. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { Could we also update the documentation on ViewHostMsg_TakeFocus while we're at it? It currently says: // Tells the browser to move the focus to the next (previous if reverse is // true) focusable element. This seems outdated - I think it should say something about advancing focus to Chrome itself if this is the top-level view, or, in the <webview> case, continue focus traversal in an outer WebContents if it exists. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { How does the other part of tab traversal work, when you need to descend from outer contents into inner contents? Does that already work today, or something you will be fixing in a followup patch? (FWIW, I tried out this patch on the "Webview Transparency" sample, with two <webviews> and each having some <input>'s, and the tab traversal wasn't going from one webview to the next, when starting from one of the inputs in either <webview>. Interestingly, it worked fine when starting from the embedder. You may want to double-check this case.) https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:157: web_contents_->GetRenderManager()->GetProxyToOuterDelegate(); It seems you can avoid making GetRenderManager public and get this via web_contents_->GetMainFrame()->frame_tree_node()->GetRenderManager(), though that's a long detour. OTOH, if you make GetRenderManager public, then you probably should also remove GetRenderManagerForTesting() (perhaps in a followup). I'd ask creis@ or nasko@ for opinions on whether it's ok to make GetRenderManager() public in web_contents_impl.h. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_guest.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_guest.cc:220: LOG(ERROR) << this << "WebContentsViewChildFrame::TakeFocus: " << reverse; Don't forget to remove. And by the way, does the fact that this is empty mean that tab focus traversal doesn't work across <webview> in non-OOPIF mode today? https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: if (!m_focusedFrame && m_isFocused) Why is this needed? How is it related to the rest of this CL? If it's needed, can you add a comment?
https://codereview.chromium.org/2474323002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:363: type, (source_rfh On 2016/11/22 02:45:27, alexmos wrote: > Let's keep source_proxy defined as a separate var and pass it here. I think it > makes the code a bit easier to follow. Done. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/11/22 02:45:27, alexmos wrote: > Could we also update the documentation on ViewHostMsg_TakeFocus while we're at > it? It currently says: > > // Tells the browser to move the focus to the next (previous if reverse is > // true) focusable element. > > This seems outdated - I think it should say something about advancing focus to > Chrome itself if this is the top-level view, or, in the <webview> case, continue > focus traversal in an outer WebContents if it exists. Done. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/11/22 02:45:27, alexmos wrote: > How does the other part of tab traversal work, when you need to descend from > outer contents into inner contents? Does that already work today, or something > you will be fixing in a followup patch? > > (FWIW, I tried out this patch on the "Webview Transparency" sample, with two > <webviews> and each having some <input>'s, and the tab traversal wasn't going > from one webview to the next, when starting from one of the inputs in either > <webview>. Interestingly, it worked fine when starting from the embedder. You > may want to double-check this case.) The problem here I think is that tabbing into the webview will have the embedder renderer show the webview as the document.activeElement. If clicking into the webview, the embedder does not know it lost focus to the webview and doesn't update the active element. In this case it doesn't know where to continue from (I think.) This seems strange since I though the source frame should have handled this. On further investigation it seems the failure is the early out here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... It seems that the next element after the webview (in the capture visible region sample, the 'screenshot' button) is focused and the one that should have gotten focus. I think that we need to focus the webview's frame before attempting to move focus to ensure that something else is not. The other option would be to clear the focused element when moving into the webview. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:157: web_contents_->GetRenderManager()->GetProxyToOuterDelegate(); On 2016/11/22 02:45:27, alexmos wrote: > It seems you can avoid making GetRenderManager public and get this via > web_contents_->GetMainFrame()->frame_tree_node()->GetRenderManager(), though > that's a long detour. OTOH, if you make GetRenderManager public, then you > probably should also remove GetRenderManagerForTesting() (perhaps in a > followup). I'd ask creis@ or nasko@ for opinions on whether it's ok to make > GetRenderManager() public in web_contents_impl.h. I'll use the detour for now. There's already a comment there saying it should be removed. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_guest.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_guest.cc:220: LOG(ERROR) << this << "WebContentsViewChildFrame::TakeFocus: " << reverse; On 2016/11/22 02:45:27, alexmos wrote: > Don't forget to remove. And by the way, does the fact that this is empty mean > that tab focus traversal doesn't work across <webview> in non-OOPIF mode today? No, ViewHostMsg_TakeFocus is handled by BrowserPluginGuest::OnTakeFocus. https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: if (!m_focusedFrame && m_isFocused) On 2016/11/22 02:45:27, alexmos wrote: > Why is this needed? How is it related to the rest of this CL? If it's needed, > can you add a comment? When you call Blur on a page with no focused frame, this ends up focusing a frame and sending back an IPC that steals focus.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good, a couple more questions and still waiting for a test. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/11/25 19:39:54, avallee wrote: > On 2016/11/22 02:45:27, alexmos wrote: > > How does the other part of tab traversal work, when you need to descend from > > outer contents into inner contents? Does that already work today, or > something > > you will be fixing in a followup patch? > > > > (FWIW, I tried out this patch on the "Webview Transparency" sample, with two > > <webviews> and each having some <input>'s, and the tab traversal wasn't going > > from one webview to the next, when starting from one of the inputs in either > > <webview>. Interestingly, it worked fine when starting from the embedder. > You > > may want to double-check this case.) > > The problem here I think is that tabbing into the webview will have the embedder > renderer show the webview as the document.activeElement. > > If clicking into the webview, the embedder does not know it lost focus to the > webview and doesn't update the active element. In this case it doesn't know > where to continue from (I think.) This seems strange since I though the source > frame should have handled this. Yes, this is indeed strange, since the early out you mention below happens after the call to findFocusableElementAcrossFocusScopes, which should've gotten the right HTMLFrameOwnerElement as the |start| element (which is computed using the source frame). Can you verify that this is indeed what's happening, and |start| is valid? > > On further investigation it seems the failure is the early out here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > It seems that the next element after the webview (in the capture visible region > sample, the 'screenshot' button) is focused and the one that should have gotten > focus. > > I think that we need to focus the webview's frame before attempting to move > focus to ensure that something else is not. The other option would be to clear > the focused element when moving into the webview. Yes, I can see how that is needed for document.activeElement. I'm still not 100% sure I see how not having the <webview> frame focused in the embedder leads to the bug above, given that we plumb in the source frame element. Clearing the focused element when moving into/out of the <webview> would be similar to what we did for OOPIFs, here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... Is moving into a <webview> for tab traversal also going to go through this code? I'm just wondering if that's any easier to do. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_guest.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_guest.cc:220: LOG(ERROR) << this << "WebContentsViewChildFrame::TakeFocus: " << reverse; On 2016/11/25 19:39:54, avallee wrote: > On 2016/11/22 02:45:27, alexmos wrote: > > Don't forget to remove. And by the way, does the fact that this is empty mean > > that tab focus traversal doesn't work across <webview> in non-OOPIF mode > today? > > No, ViewHostMsg_TakeFocus is handled by BrowserPluginGuest::OnTakeFocus. Acknowledged. https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: if (!m_focusedFrame && m_isFocused) On 2016/11/25 19:39:54, avallee wrote: > On 2016/11/22 02:45:27, alexmos wrote: > > Why is this needed? How is it related to the rest of this CL? If it's > needed, > > can you add a comment? > > When you call Blur on a page with no focused frame, this ends up focusing a > frame and sending back an IPC that steals focus. In what scenario would that happen during tab traversal? I thought we got rid of unsetting the focused frame in https://codereview.chromium.org/2451143003/, so then is this only happening when blurring a WebContents on which you've never clicked (which thus has no focused frame to start with)? https://codereview.chromium.org/2474323002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2474323002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4681: if (GetRenderManager()->GetProxyToOuterDelegate()) { nit: { not necessary https://codereview.chromium.org/2474323002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: // Do not set a focused frame when being unfocsed. This might reset focus to nits: s/unfocsed/unfocused/ s/reset focus/reset m_isFocused/ (this is what you meant, right?)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/11/28 23:52:23, alexmos wrote: > On 2016/11/25 19:39:54, avallee wrote: > > On 2016/11/22 02:45:27, alexmos wrote: > > > How does the other part of tab traversal work, when you need to descend from > > > outer contents into inner contents? Does that already work today, or > > something > > > you will be fixing in a followup patch? > > > > > > (FWIW, I tried out this patch on the "Webview Transparency" sample, with two > > > <webviews> and each having some <input>'s, and the tab traversal wasn't > going > > > from one webview to the next, when starting from one of the inputs in either > > > <webview>. Interestingly, it worked fine when starting from the embedder. > > You > > > may want to double-check this case.) > > > > The problem here I think is that tabbing into the webview will have the > embedder > > renderer show the webview as the document.activeElement. > > > > If clicking into the webview, the embedder does not know it lost focus to the > > webview and doesn't update the active element. In this case it doesn't know > > where to continue from (I think.) This seems strange since I though the source > > frame should have handled this. > > Yes, this is indeed strange, since the early out you mention below happens after > the call to findFocusableElementAcrossFocusScopes, which should've gotten the > right HTMLFrameOwnerElement as the |start| element (which is computed using the > source frame). Can you verify that this is indeed what's happening, and |start| > is valid? On further investigation, this is what happens. Oddly enough, when clicking into the webview, the webview will become the focused/active element in some cases. For example, in the sample app, if the body is the active element, clicking the body of the webview will focus the webview element in the embedder. If there was a button focused, the button stays focused. When tabbing into the webview, the embedder will go through the code path you mention above and clear the focused element in the embedder document. Now if the screenshot button happens to be focused and you click the webview, you'll now be in the webview and tabbing through its elements. However, clicking the webview leaves the button element focused in the embedder. Then, when we focus forward past the end of the webview contents, that's when we ask the embedder to continue focus and it starts from the right place and then decides that the button is the next element correctly. However, it decides to early out since it notices the button is already focused. Yes, this is called fromn advanceFocusAcrossFrames with the start element set to the HTMLFrameOwnerElement(fromFrame->Owner()); This also seems to be broken with oopifs. > > > > > On further investigation it seems the failure is the early out here: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > > > It seems that the next element after the webview (in the capture visible > region > > sample, the 'screenshot' button) is focused and the one that should have > gotten > > focus. > > > > I think that we need to focus the webview's frame before attempting to move > > focus to ensure that something else is not. The other option would be to clear > > the focused element when moving into the webview. > > Yes, I can see how that is needed for document.activeElement. I'm still not > 100% sure I see how not having the <webview> frame focused in the embedder leads > to the bug above, given that we plumb in the source frame element. > > Clearing the focused element when moving into/out of the <webview> would be > similar to what we did for OOPIFs, here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > Is moving into a <webview> for tab traversal also going to go through this code? > I'm just wondering if that's any easier to do. Yes, it looks like tabbing into the webview goes through this code path. https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: if (!m_focusedFrame && m_isFocused) On 2016/11/28 23:52:24, alexmos (OOO until 12-8) wrote: > On 2016/11/25 19:39:54, avallee wrote: > > On 2016/11/22 02:45:27, alexmos wrote: > > > Why is this needed? How is it related to the rest of this CL? If it's > > needed, > > > can you add a comment? > > > > When you call Blur on a page with no focused frame, this ends up focusing a > > frame and sending back an IPC that steals focus. > > In what scenario would that happen during tab traversal? I thought we got rid > of unsetting the focused frame in https://codereview.chromium.org/2451143003/, > so then is this only happening when blurring a WebContents on which you've never > clicked (which thus has no focused frame to start with)? When we reach the end of the guest focusable elements we ask our embedder to TakeFocus after clearing the focused element and focused frame: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... As part of taking focus from the guest we blur the widget in which case we arrive here with no focused frame and isFocused false. https://codereview.chromium.org/2474323002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2474323002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4681: if (GetRenderManager()->GetProxyToOuterDelegate()) { On 2016/11/28 23:52:24, alexmos (OOO until 12-8) wrote: > nit: { not necessary Done.
Thanks for the explanations. LGTM, though it looks like a couple of the re-enabled tests are still failing on the trybots? https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/12/09 21:16:51, avallee wrote: > On 2016/11/28 23:52:23, alexmos wrote: > > On 2016/11/25 19:39:54, avallee wrote: > > > On 2016/11/22 02:45:27, alexmos wrote: > > > > How does the other part of tab traversal work, when you need to descend > from > > > > outer contents into inner contents? Does that already work today, or > > > something > > > > you will be fixing in a followup patch? > > > > > > > > (FWIW, I tried out this patch on the "Webview Transparency" sample, with > two > > > > <webviews> and each having some <input>'s, and the tab traversal wasn't > > going > > > > from one webview to the next, when starting from one of the inputs in > either > > > > <webview>. Interestingly, it worked fine when starting from the embedder. > > > > You > > > > may want to double-check this case.) > > > > > > The problem here I think is that tabbing into the webview will have the > > embedder > > > renderer show the webview as the document.activeElement. > > > > > > If clicking into the webview, the embedder does not know it lost focus to > the > > > webview and doesn't update the active element. In this case it doesn't know > > > where to continue from (I think.) This seems strange since I though the > source > > > frame should have handled this. > > > > Yes, this is indeed strange, since the early out you mention below happens > after > > the call to findFocusableElementAcrossFocusScopes, which should've gotten the > > right HTMLFrameOwnerElement as the |start| element (which is computed using > the > > source frame). Can you verify that this is indeed what's happening, and > |start| > > is valid? > > On further investigation, this is what happens. Oddly enough, when clicking into > the webview, the webview will become the focused/active element in some cases. > For example, in the sample app, if the body is the active element, clicking the > body of the webview will focus the webview element in the embedder. If there was > a button focused, the button stays focused. > > When tabbing into the webview, the embedder will go through the code path you > mention above and clear the focused element in the embedder document. Now if the > screenshot button happens to be focused and you click the webview, you'll now be > in the webview and tabbing through its elements. However, clicking the webview > leaves the button element focused in the embedder. Then, when we focus forward > past the end of the webview contents, that's when we ask the embedder to > continue focus and it starts from the right place and then decides that the > button is the next element correctly. However, it decides to early out since it > notices the button is already focused. > > Yes, this is called fromn advanceFocusAcrossFrames with the start element set to > the HTMLFrameOwnerElement(fromFrame->Owner()); > > This also seems to be broken with oopifs. Wow, good find, thanks for the investigation. Indeed, I've reproed this with OOPIFs: 1. go to http://csreis.github.io/tests/cross-site-iframe-simple.html with --site-per-process 2. from devtools, document.body.appendChild(document.createElement("input")) 3. click on the new <input> 4. click on an <input> in the OOPIF, then keep tabbing. When reaching the end, focus should shift to the new <input> in the main frame, but instead it wraps back into the OOPIF. Can you please file a bug with these steps, along with the webview repro steps? I checked that document.activeElement is also wrong -- it should be pointing at the <iframe>/<webview> element after the click in step 4, but it keeps pointing at the old <input>. No need to fix this here, but I can take a look at the OOPIF side of this in a followup. I think we just need to update activeElement to the right HTMLFrameOwnerElement in this case, when we receive a message in the parent frame that a RemoteFrame is focused. https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:877: if (!m_focusedFrame && m_isFocused) On 2016/12/09 21:16:51, avallee wrote: > On 2016/11/28 23:52:24, alexmos (OOO until 12-8) wrote: > > On 2016/11/25 19:39:54, avallee wrote: > > > On 2016/11/22 02:45:27, alexmos wrote: > > > > Why is this needed? How is it related to the rest of this CL? If it's > > > needed, > > > > can you add a comment? > > > > > > When you call Blur on a page with no focused frame, this ends up focusing a > > > frame and sending back an IPC that steals focus. > > > > In what scenario would that happen during tab traversal? I thought we got rid > > of unsetting the focused frame in https://codereview.chromium.org/2451143003/, > > so then is this only happening when blurring a WebContents on which you've > never > > clicked (which thus has no focused frame to start with)? > > When we reach the end of the guest focusable elements we ask our embedder to > TakeFocus after clearing the focused element and focused frame: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > As part of taking focus from the guest we blur the widget in which case we > arrive here with no focused frame and isFocused false. Acknowledged. I didn't realize that the TakeFocus path clears the focused frame.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
avallee@chromium.org changed reviewers: + dcheng@chromium.org, kenrb@chromium.org, wjmaclean@chromium.org
wjmaclean: chrome/ kenrb: ipc comment rubber stamp. dcheng: blink Hi alexmos@ I think that the flakiness for the tests is finally fixed. There was a race between the input being sent to a renderer in enough burst that everything is queued to the embedder before the browser learns that the guest is focused. Added explicit check for a frame getting focused in one case, and a click being routed as it would in the actual browser. https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_child_frame.cc (right): https://codereview.chromium.org/2474323002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_child_frame.cc:155: void WebContentsViewChildFrame::TakeFocus(bool reverse) { On 2016/12/12 22:41:15, alexmos wrote: > On 2016/12/09 21:16:51, avallee wrote: > > On 2016/11/28 23:52:23, alexmos wrote: > > > On 2016/11/25 19:39:54, avallee wrote: > > > > On 2016/11/22 02:45:27, alexmos wrote: > > > > > How does the other part of tab traversal work, when you need to descend > > from > > > > > outer contents into inner contents? Does that already work today, or > > > > something > > > > > you will be fixing in a followup patch? > > > > > > > > > > (FWIW, I tried out this patch on the "Webview Transparency" sample, with > > two > > > > > <webviews> and each having some <input>'s, and the tab traversal wasn't > > > going > > > > > from one webview to the next, when starting from one of the inputs in > > either > > > > > <webview>. Interestingly, it worked fine when starting from the > embedder. > > > > > > You > > > > > may want to double-check this case.) > > > > > > > > The problem here I think is that tabbing into the webview will have the > > > embedder > > > > renderer show the webview as the document.activeElement. > > > > > > > > If clicking into the webview, the embedder does not know it lost focus to > > the > > > > webview and doesn't update the active element. In this case it doesn't > know > > > > where to continue from (I think.) This seems strange since I though the > > source > > > > frame should have handled this. > > > > > > Yes, this is indeed strange, since the early out you mention below happens > > after > > > the call to findFocusableElementAcrossFocusScopes, which should've gotten > the > > > right HTMLFrameOwnerElement as the |start| element (which is computed using > > the > > > source frame). Can you verify that this is indeed what's happening, and > > |start| > > > is valid? > > > > On further investigation, this is what happens. Oddly enough, when clicking > into > > the webview, the webview will become the focused/active element in some cases. > > For example, in the sample app, if the body is the active element, clicking > the > > body of the webview will focus the webview element in the embedder. If there > was > > a button focused, the button stays focused. > > > > When tabbing into the webview, the embedder will go through the code path you > > mention above and clear the focused element in the embedder document. Now if > the > > screenshot button happens to be focused and you click the webview, you'll now > be > > in the webview and tabbing through its elements. However, clicking the webview > > leaves the button element focused in the embedder. Then, when we focus forward > > past the end of the webview contents, that's when we ask the embedder to > > continue focus and it starts from the right place and then decides that the > > button is the next element correctly. However, it decides to early out since > it > > notices the button is already focused. > > > > Yes, this is called fromn advanceFocusAcrossFrames with the start element set > to > > the HTMLFrameOwnerElement(fromFrame->Owner()); > > > > This also seems to be broken with oopifs. > > Wow, good find, thanks for the investigation. Indeed, I've reproed this with > OOPIFs: > > 1. go to http://csreis.github.io/tests/cross-site-iframe-simple.html with > --site-per-process > 2. from devtools, document.body.appendChild(document.createElement("input")) > 3. click on the new <input> > 4. click on an <input> in the OOPIF, then keep tabbing. When reaching the end, > focus should shift to the new <input> in the main frame, but instead it wraps > back into the OOPIF. > > Can you please file a bug with these steps, along with the webview repro steps? > I checked that document.activeElement is also wrong -- it should be pointing at > the <iframe>/<webview> element after the click in step 4, but it keeps pointing > at the old <input>. No need to fix this here, but I can take a look at the > OOPIF side of this in a followup. I think we just need to update activeElement > to the right HTMLFrameOwnerElement in this case, when we receive a message in > the parent frame that a RemoteFrame is focused. Done. Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=674219
Description was changed from ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. ~ Reenable 8 interactive browsertests for webview when using OOPIF. ~ Fix flakiness in OOPIF-webview versions of WebViewFocusInteractiveTest.FocusAndVisibility and WebViewFocusInteractiveTest.Focus_AdvanceFocus. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ipc lgtm, although not really necessary because dcheng is also an IPC owner.
On 2017/01/11 16:18:10, kenrb wrote: > ipc lgtm, although not really necessary because dcheng is also an IPC owner. chrome/ lgtm
(sorry for the delayed review) https://codereview.chromium.org/2474323002/diff/200001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/2474323002/diff/200001/content/common/view_me... content/common/view_messages.h:782: // focusable element in chrome if this is the top-level view or tells an outer This comment is a bit confusing to me. I assume "chrome" here means the chrome ui, and if I weren't familiar with OOPIF, I would be wondering why this comment mentions this outer/inner web contents concept at all. Also, I would imagine this is also used when focus transitions between cross-process frames, right? Perhaps wording the comment to be a more generic would be nice: When the renderer needs the browser to transfer focus cross-process on its behalf in the focus hierarchy. This may focus an element in the browser ui or a cross-process frame, as appropriate. https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:860: if (!m_focusedFrame && m_isFocused) This seems like a reasonable change to me, but I'm wondering if it's web visible at all?
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
https://codereview.chromium.org/2474323002/diff/200001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/2474323002/diff/200001/content/common/view_me... content/common/view_messages.h:782: // focusable element in chrome if this is the top-level view or tells an outer On 2017/01/12 10:19:21, dcheng wrote: > This comment is a bit confusing to me. I assume "chrome" here means the chrome > ui, and if I weren't familiar with OOPIF, I would be wondering why this comment > mentions this outer/inner web contents concept at all. > > Also, I would imagine this is also used when focus transitions between > cross-process frames, right? > > Perhaps wording the comment to be a more generic would be nice: > When the renderer needs the browser to transfer focus cross-process on its > behalf in the focus hierarchy. This may focus an element in the browser ui or a > cross-process frame, as appropriate. Done. https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:860: if (!m_focusedFrame && m_isFocused) On 2017/01/12 10:19:21, dcheng wrote: > This seems like a reasonable change to me, but I'm wondering if it's web visible > at all? See the conversation here: https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the long latency, I thought I sent out my drafts =( https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:860: if (!m_focusedFrame && m_isFocused) On 2017/01/12 20:00:23, avallee wrote: > On 2017/01/12 10:19:21, dcheng wrote: > > This seems like a reasonable change to me, but I'm wondering if it's web > visible > > at all? > > See the conversation here: > https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... Would it be better to just pass false for notifyEmbedder here? After reading this code, I haven't managed to convince myself this is OK. One of the places that calls this function is WebViewImpl::setFocus(), and there's a fair amount of work that's performed if there's a focused frame. How do we end up here without any focused frames at all anyway?
dcheng: PTAL. https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:860: if (!m_focusedFrame && m_isFocused) On 2017/01/20 07:02:38, dcheng wrote: > On 2017/01/12 20:00:23, avallee wrote: > > On 2017/01/12 10:19:21, dcheng wrote: > > > This seems like a reasonable change to me, but I'm wondering if it's web > > visible > > > at all? > > > > See the conversation here: > > > https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... > > Would it be better to just pass false for notifyEmbedder here? After reading > this code, I haven't managed to convince myself this is OK. One of the places > that calls this function is WebViewImpl::setFocus(), and there's a fair amount > of work that's performed if there's a focused frame. > > How do we end up here without any focused frames at all anyway? Oh, i thought I had previously linked to the spot that clears the frame. If you look here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... In the oopif case, we would hit the previous if block to advance focus out to the embedder frame: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... In the webview case, the page thinks it exists by itself, it's a main frame and has reached the end of the content. In this case it returns focus to the "Chrome" layer (just a different embedder).
On 2017/01/20 14:19:11, avallee wrote: > dcheng: PTAL. > > https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/FocusController.cpp (right): > > https://codereview.chromium.org/2474323002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/FocusController.cpp:860: if (!m_focusedFrame > && m_isFocused) > On 2017/01/20 07:02:38, dcheng wrote: > > On 2017/01/12 20:00:23, avallee wrote: > > > On 2017/01/12 10:19:21, dcheng wrote: > > > > This seems like a reasonable change to me, but I'm wondering if it's web > > > visible > > > > at all? > > > > > > See the conversation here: > > > > > > https://codereview.chromium.org/2474323002/diff/60001/third_party/WebKit/Sour... > > > > Would it be better to just pass false for notifyEmbedder here? After reading > > this code, I haven't managed to convince myself this is OK. One of the places > > that calls this function is WebViewImpl::setFocus(), and there's a fair amount > > of work that's performed if there's a focused frame. > > > > How do we end up here without any focused frames at all anyway? > > Oh, i thought I had previously linked to the spot that clears the frame. If you > look here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > In the oopif case, we would hit the previous if block to advance focus out to > the embedder frame: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... > > In the webview case, the page thinks it exists by itself, it's a main frame and > has reached the end of the content. In this case it returns focus to the > "Chrome" layer (just a different embedder). Hmm. LGTM. It's not clear to me that this is completely correct, but the original CL looks suspect as well: https://chromium.googlesource.com/chromium/src/+/cd3ecabfabb0be5558631d1d1b78... I'll try to clean this up in a followup and see what happens, I guess.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, kenrb@chromium.org, wjmaclean@chromium.org Link to the patchset: https://codereview.chromium.org/2474323002/#ps220001 (title: "Fix header comment as requested by dcheng.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:514
error: chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:
patch does not apply
Patch: chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
Index: chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
diff --git a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
index
332add5cdfe42624c708009139875da972af4b99..ce963806468ee9c95460eb531fcb704ae336f0aa
100644
--- a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
+++ b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
@@ -514,6 +514,7 @@ class WebViewInteractiveTest : public
WebViewInteractiveTestBase,
};
class WebViewNewWindowInteractiveTest : public WebViewInteractiveTest {};
+class WebViewFocusInteractiveTest : public WebViewInteractiveTest {};
class WebViewPointerLockInteractiveTest : public WebViewInteractiveTest {};
// The tests below aren't needed in --use-cross-process-frames-for-guests.
@@ -522,7 +523,6 @@ class WebViewContextMenuInteractiveTest : public
WebViewInteractiveTestBase {};
// The following class of tests do not work for OOPIF <webview>.
// TODO(ekaramad): Make this tests work with OOPIF and replace the test classes
// with WebViewInteractiveTest (see crbug.com/582562).
-class WebViewFocusInteractiveTest : public WebViewInteractiveTestBase {};
class WebViewPopupInteractiveTest : public WebViewInteractiveTestBase {};
class WebViewDragDropInteractiveTest : public WebViewInteractiveTestBase {};
@@ -535,6 +535,10 @@ INSTANTIATE_TEST_CASE_P(WebViewInteractiveTests,
testing::Bool());
INSTANTIATE_TEST_CASE_P(WebViewInteractiveTests,
+ WebViewFocusInteractiveTest,
+ testing::Bool());
+
+INSTANTIATE_TEST_CASE_P(WebViewInteractiveTests,
WebViewPointerLockInteractiveTest,
testing::Bool());
@@ -648,17 +652,21 @@ IN_PROC_BROWSER_TEST_P(WebViewPointerLockInteractiveTest,
// Tests that if a <webview> is focused before navigation then the guest starts
// off focused.
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest,
Focus_FocusBeforeNavigation) {
+ // TODO(avallee): Determine if test is relevant with OOPIF or fix the bug.
+ // http://crbug.com/672947
+ if (GetParam())
+ return;
TestHelper("testFocusBeforeNavigation", "web_view/focus", NO_TEST_SERVER);
}
// Tests that setting focus on the <webview> sets focus on the guest.
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, Focus_FocusEvent) {
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusEvent) {
TestHelper("testFocusEvent", "web_view/focus", NO_TEST_SERVER);
}
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, Focus_FocusTracksEmbedder)
{
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusTracksEmbedder)
{
content::WebContents* embedder_web_contents = NULL;
std::unique_ptr<ExtensionTestMessageListener> done_listener(
@@ -678,7 +686,7 @@ IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
Focus_FocusTracksEmbedder) {
ASSERT_TRUE(next_step_listener.WaitUntilSatisfied());
}
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, Focus_AdvanceFocus) {
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_AdvanceFocus) {
content::WebContents* embedder_web_contents = NULL;
{
@@ -691,9 +699,16 @@ IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
Focus_AdvanceFocus) {
{
ExtensionTestMessageListener listener("button1-focused", false);
listener.set_failure_message("TEST_FAILED");
- SimulateRWHMouseClick(
- embedder_web_contents->GetRenderViewHost()->GetWidget(),
- blink::WebMouseEvent::Button::Left, 200, 20);
+
+ // In oopif-webview, the click it directly routed to the guest.
+ content::WebContents* guest =
+ GetParam() ? GetGuestViewManager()->WaitForSingleGuestCreated()
+ : nullptr;
+
+ SimulateRWHMouseClick((guest ? guest : embedder_web_contents)
+ ->GetRenderViewHost()
+ ->GetWidget(),
+ blink::WebMouseEvent::Button::Left, 200, 20);
content::SimulateKeyPress(embedder_web_contents, ui::DomKey::TAB,
ui::DomCode::TAB, ui::VKEY_TAB, false, false,
false, false);
@@ -724,7 +739,7 @@ IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
Focus_AdvanceFocus) {
}
// Tests that blurring <webview> also blurs the guest.
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, Focus_BlurEvent) {
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_BlurEvent) {
TestHelper("testBlurEvent", "web_view/focus", NO_TEST_SERVER);
}
@@ -1159,7 +1174,7 @@ IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest,
// Now we need to make sure TextInputTypeChanged fires properly for the guest's
// view upon step #3. We simply read the input type's state after #3 to
// make sure it's not TEXT_INPUT_TYPE_NONE.
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, Focus_FocusRestored) {
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusRestored) {
TestHelper("testFocusRestored", "web_view/focus", NO_TEST_SERVER);
content::WebContents* embedder_web_contents = GetFirstAppWindowWebContents();
ASSERT_TRUE(embedder_web_contents);
@@ -1336,7 +1351,7 @@ IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest,
TextSelection) {
#define MAYBE_FocusAndVisibility FocusAndVisibility
#endif
-IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest, MAYBE_FocusAndVisibility) {
+IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, MAYBE_FocusAndVisibility) {
ASSERT_TRUE(StartEmbeddedTestServer());
LoadAndLaunchPlatformApp("web_view/focus_visibility",
"WebViewInteractiveTest.LOADED");
@@ -1344,9 +1359,24 @@ IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
MAYBE_FocusAndVisibility) {
"WebViewInteractiveTest.WebViewInitialized", false);
SendMessageToEmbedder("init");
test_init_listener.WaitUntilSatisfied();
+
+ // In oopif-webview, wait until the tab key triggers a focus change.
+ std::unique_ptr<content::FrameFocusedObserver> frame_focus_observer =
+ GetParam() ? base::MakeUnique<content::FrameFocusedObserver>(
+ GetGuestViewManager()
+ ->WaitForSingleGuestCreated()
+ ->GetMainFrame())
+ : nullptr;
+
// Send several tab-keys. The button inside webview should receive focus at
// least once.
- for (size_t i = 0; i < 4; ++i)
+ for (size_t i = 0; i < 2; ++i)
+ SendKeyPressToPlatformApp(ui::VKEY_TAB);
+ if (frame_focus_observer) {
+ frame_focus_observer->Wait();
+ frame_focus_observer.reset();
+ }
+ for (size_t i = 0; i < 2; ++i)
SendKeyPressToPlatformApp(ui::VKEY_TAB);
ExtensionTestMessageListener webview_button_focused_listener(
"WebViewInteractiveTest.WebViewButtonWasFocused", false);
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, wjmaclean@chromium.org, alexmos@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2474323002/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484957200698420,
"parent_rev": "1308503cb592bad406c0115fa102e674623fe365", "commit_rev":
"c2ff1efb3496708d94fb12f79b3aeac6b1846b4b"}
Message was sent while issue was closed.
Description was changed from ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. ~ Reenable 8 interactive browsertests for webview when using OOPIF. ~ Fix flakiness in OOPIF-webview versions of WebViewFocusInteractiveTest.FocusAndVisibility and WebViewFocusInteractiveTest.Focus_AdvanceFocus. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement WebContentsViewChildFrame::TakeFocus. TakeFocus will take focus from the child and proceed to sequentially move focus in the parent WebContents. + Implement TakeFocus() ~ Move bulk of logic sending FrameMsg_AdvanceFocus to RenderFrame from RenderFrameProxyHost to RenderFrameHostImpl. ~ Prevent blink's FocusController from focusing the main frame if no frame is focused on a blur event. ~ Reenable 8 interactive browsertests for webview when using OOPIF. ~ Fix flakiness in OOPIF-webview versions of WebViewFocusInteractiveTest.FocusAndVisibility and WebViewFocusInteractiveTest.Focus_AdvanceFocus. BUG=610819 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2474323002 Cr-Commit-Position: refs/heads/master@{#445221} Committed: https://chromium.googlesource.com/chromium/src/+/c2ff1efb3496708d94fb12f79b3a... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/c2ff1efb3496708d94fb12f79b3a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
