Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674353003:
Enable WebViewAccessiblityTest for OOPIF webview. (Closed)
DescriptionEnable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795, 685739
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2674353003
Cr-Commit-Position: refs/heads/master@{#481326}
Committed: https://chromium.googlesource.com/chromium/src/+/d68cdd1afdff55d24558ef88441d9ac950d46dd4
Patch Set 1 #Patch Set 2 : Accessiblity tree will search for inner web contents. #
Total comments: 8
Patch Set 3 : Address dmazzoni comments. #
Total comments: 6
Patch Set 4 : Fixed comment and spacing. #
Total comments: 20
Patch Set 5 : Address alexmos comments. #Patch Set 6 : remove change from unrelated cl. #
Total comments: 10
Patch Set 7 : Rebase #Patch Set 8 : Instrument and fix #Patch Set 9 : Remove false assert #Patch Set 10 : Remove test logging. #Patch Set 11 : Address alexmos final comments. #
Total comments: 3
Patch Set 12 : Fix typos. #
Messages
Total messages: 84 (61 generated)
avallee@chromium.org changed reviewers: + dmazzoni@chromium.org
Is WebContentsImpl the right place to do this or would GuestView/GuestViewManager/RenderFrameImpl feel more natural? Dmazzoni: Let me know what you think of this. I initially tried to amend the check to if child parent is this or child parent is null, but it feels dirtier.
lgtm This makes sense. Do you think we should rename browser_plugin_embedder_ax_tree_id to something more general or more correct since the new oopif-based webview doesn't use a browser plugin?
As for fixing focus, there are two possibilities: 1. As you noticed, RenderFrameHostImpl::AXContentTreeDataToAXTreeData is one place where we try to set the id of the focused frame. If you can fix it here, you'll need to call UpdateAXTreeData on the root frame when a webview gets or loses focus. 2. Alternatively, BrowserAccessibilityManager::GetFocusFromThisOrDescendantFrame() is supposed to recursively look for focused elements inside of subframes or guest views by looking for the child frame id. I'd suggest checking why this isn't working now.
On 2017/02/06 18:59:12, dmazzoni wrote: > lgtm > > This makes sense. > > Do you think we should rename > browser_plugin_embedder_ax_tree_id to something > more general or more correct since the new > oopif-based webview doesn't use a browser plugin? There is still a BrowserPluginEmbedder/BrowserPluginGuest associated with the WebContents, but in the oopif case, they are pretty stripped. Those classes probably need some refactoring/renaming. Any hints as to why FocusAccessibility would hang? Currently investigating to see if I can fix it simply or need to disable until a follow up cl.
Description was changed from
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
WebViewAccessibilityTest.FocusAccessibility still currently hangs.
BUG=610795
==========
to
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
WebViewAccessibilityTest.FocusAccessibility still currently hangs.
BUG=610795
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...
Dmazzoni: ptal, I fixed the test that was hanging. Lfg: any comment on the approach? Like thing at the wrong level of abstraction.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:890: testing::Bool()); Yay! I think you can update the change description now that this doesn't hang. https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:227: // Gets the focused frame from possibly another inner WebContents that The comment says "focused" but the name of the function and the implementation don't really have anything to do with "focused" as far as I can tell... After reading through the impl in WebContents I'm still not really clear what this does conceptually. Can you come up with a more clear name for this? I'd prefer a longer more verbose name over a short one since it's pretty obscure and only used in one place. https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3335: delegate_->GetFrameAtNode(focused_frame_tree_node)); Maybe it'd be cleaner if you called it GetFocusedFrameIncludingGuestWebContents or something like that and had it take no arguments rather than taking in a FrameTreeNode that's only halfway helpful. https://codereview.chromium.org/2674353003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4734: while (contents && Maybe this could be a helper function, like FindOutermostWebContents?
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
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
WebViewAccessibilityTest.FocusAccessibility still currently hangs.
BUG=610795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enable WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
WebViewAccessibilityTest.FocusAccessibility still currently hangs.
BUG=610795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
Description was changed from
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enable WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
WebViewAccessibilityTest.FocusAccessibility still currently hangs.
BUG=610795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
Done. https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:890: testing::Bool()); On 2017/02/16 00:19:01, dmazzoni wrote: > Yay! > > I think you can update the change description > now that this doesn't hang. Done. https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:227: // Gets the focused frame from possibly another inner WebContents that On 2017/02/16 00:19:02, dmazzoni wrote: > The comment says "focused" but the name of the function and > the implementation don't really have anything to do with > "focused" as far as I can tell... > > After reading through the impl in WebContents I'm still not > really clear what this does conceptually. Can you come up with > a more clear name for this? I'd prefer a longer more verbose > name over a short one since it's pretty obscure and only used > in one place. Done. Got a longer, more descriptive, name. Also, expanded the comment. https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3335: delegate_->GetFrameAtNode(focused_frame_tree_node)); On 2017/02/16 00:19:02, dmazzoni wrote: > Maybe it'd be cleaner if you called it GetFocusedFrameIncludingGuestWebContents > or something like that > and had it take no arguments rather than taking in a FrameTreeNode > that's only halfway helpful. Reworked to just navigate down the WebContents tree and finding one RenderFrameHost (or null if no focus in the top WebContents). https://codereview.chromium.org/2674353003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4734: while (contents && On 2017/02/16 00:19:02, dmazzoni wrote: > Maybe this could be a helper function, like > FindOutermostWebContents? Done.
avallee@chromium.org changed reviewers: + wjmaclean@chromium.org
wjmaclean@chromium.org: Please review changes in chrome/
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm Very clear now! https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:232: // frame, return the main frame, since the attchment frame in its outer nit: attchment -> attachment. Also, "return the main frame" should be "return its main frame" since it will return the main frame of the inner WebContents not the main frame of this. https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4341: if(GetRenderManager()->GetProxyToOuterDelegate()) nit: space before first ( https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4786: while(true) { nit: space before (
https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:232: // frame, return the main frame, since the attchment frame in its outer On 2017/02/22 08:46:41, dmazzoni wrote: > nit: attchment -> attachment. > > Also, "return the main frame" should be "return its main frame" > since it will return the main frame of the inner WebContents > not the main frame of this. Done. https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4341: if(GetRenderManager()->GetProxyToOuterDelegate()) On 2017/02/22 08:46:41, dmazzoni wrote: > nit: space before first ( Done. https://codereview.chromium.org/2674353003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4786: while(true) { On 2017/02/22 08:46:41, dmazzoni wrote: > nit: space before ( Done.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2674353003/#ps60001 (title: "Fixed comment and spacing.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
avallee@chromium.org changed reviewers: + alexmos@chromium.org
alexmos: PTAL for owners.
Thanks, a few comments and clarification questions below. It's a bit concerning that the focus logic in WebContents keeps getting more complicated; it'd be good to think about ways to simplify all this. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:427: WebContentsImpl* WebContentsImpl::WebContentsTreeNode::find_contents_at_node( Name this FindContentsAtNode, since it's not a simple getter/setter. Or better yet, FindInnerWebContentsAtNode to clarify exactly what it's getting. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1455: this, static_cast<WebContentsImpl*>(outer_web_contents), Maybe it would help if WebContentsTreeNode just knew about the WebContents it's associated with? Just something to think about for future cleanup. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4357: outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr); Can you please explain why this is now necessary? I don't see how it's related to the other changes. Another issue is that this can be called from WCI::SetFocusedFrame, and I'm concerned that we'd be losing the source SiteInstance that's passed there if we just use nullptr here. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4789: WebContentsImpl* contents_to_focus = inner_contents ? inner_contents : this; I'm a bit confused about this change also. See my earlier question about the "SetFocusedFrame(outer_node, nullptr)" above, which I'm guessing is how you would get here for the case with a valid inner_contents? Why does this become necessary? Suppose I click on a <webview>. As part of WCI::SetFocusedFrame in the inner contents, we call SetAsFocusedWebContentsIfNecessary on the inner contents. That now calls the new FocusOuterAttachmentFrameChain, which calls SetFocusedFrame(outer_node, nullptr), which ends up here and calls SetAsFocusedWebContentsIfNecessary on inner_contents again, which seems unnecessary? https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4798: auto* focused_node = contents->frame_tree_.GetFocusedFrame(); nit: at least for me, an explicit type here instead of auto would be more readable, so it's obvious that this is a FrameTreeNode* (and different from RFHI* that the other GetFocusedFrame() returns above). https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4809: focused_node->frame_tree_node_id()); Why not just pass in the FTN* and retrieve the FTN ID internally in that helper? https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4815: return contents->GetMainFrame(); What makes this necessary? How can we get here in practice? https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:883: // List of inner WebContents that we host. Mention that this is now indexed by FrameTreeNode ID.
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...
Alexmos: I'm looking at a follow up cl to delete the focused webcontents pointer and implementing GetFocusedWebContents as a walk down the focused frames if there exists an inner web contents there. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:427: WebContentsImpl* WebContentsImpl::WebContentsTreeNode::find_contents_at_node( On 2017/02/23 20:07:04, alexmos wrote: > Name this FindContentsAtNode, since it's not a simple getter/setter. Or better > yet, FindInnerWebContentsAtNode to clarify exactly what it's getting. Done. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1455: this, static_cast<WebContentsImpl*>(outer_web_contents), On 2017/02/23 20:07:04, alexmos wrote: > Maybe it would help if WebContentsTreeNode just knew about the WebContents it's > associated with? Just something to think about for future cleanup. My original iteration of the downward link did just this, but then nodes are 1 pointer larger each. Walking down focused frame tree nodes, we first look at the WebContents anyways and then its node_. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4357: outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr); On 2017/02/23 20:07:04, alexmos wrote: > Can you please explain why this is now necessary? I don't see how it's related > to the other changes. > > Another issue is that this can be called from WCI::SetFocusedFrame, and I'm > concerned that we'd be losing the source SiteInstance that's passed there if we > just use nullptr here. Maybe the site instance still needs to be passed along, I'm not quite sure. Note that this is FrameTree::SetFocusedFrame, not WebContentsImpl. For accessibility, each parent needs to correctly show the container frame tree node of the inner contents as focused so that the BrowserAccessibility/BrowserAccessibilityManager can walk down and determine which one is focused. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4789: WebContentsImpl* contents_to_focus = inner_contents ? inner_contents : this; On 2017/02/23 20:07:04, alexmos wrote: > I'm a bit confused about this change also. See my earlier question about the > "SetFocusedFrame(outer_node, nullptr)" above, which I'm guessing is how you > would get here for the case with a valid inner_contents? Why does this become > necessary? > > Suppose I click on a <webview>. As part of WCI::SetFocusedFrame in the inner > contents, we call SetAsFocusedWebContentsIfNecessary on the inner contents. > That now calls the new FocusOuterAttachmentFrameChain, which calls > SetFocusedFrame(outer_node, nullptr), which ends up here and calls > SetAsFocusedWebContentsIfNecessary on inner_contents again, which seems > unnecessary? If you clicked the webview before routing the input there would be a call to WebContentsImpl::FocusOwningWebContents which would focus the inner contents and then the frame in the outer contents, but that wouldn't call back down here. If the outer WebContents decided to focus the frame with an inner contents, then we need to tell the inner contents that it is now active which is why we move downwards like this. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4798: auto* focused_node = contents->frame_tree_.GetFocusedFrame(); On 2017/02/23 20:07:04, alexmos wrote: > nit: at least for me, an explicit type here instead of auto would be more > readable, so it's obvious that this is a FrameTreeNode* (and different from > RFHI* that the other GetFocusedFrame() returns above). Done. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4809: focused_node->frame_tree_node_id()); On 2017/02/23 20:07:04, alexmos wrote: > Why not just pass in the FTN* and retrieve the FTN ID internally in that helper? Done. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4815: return contents->GetMainFrame(); On 2017/02/23 20:07:04, alexmos wrote: > What makes this necessary? How can we get here in practice? On attach or after reaching the end of content, there is no focused frame. Should I log a warning here if you think this is unexpected? https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:883: // List of inner WebContents that we host. On 2017/02/23 20:07:05, alexmos wrote: > Mention that this is now indexed by FrameTreeNode ID. Done.
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
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795, 685739
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.
Sorry for the delay! LGTM with nits and one more question about returning the main frame with no focused frames in inner WebContents. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4357: outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr); On 2017/02/28 20:13:01, avallee wrote: > On 2017/02/23 20:07:04, alexmos wrote: > > Can you please explain why this is now necessary? I don't see how it's > related > > to the other changes. > > > > Another issue is that this can be called from WCI::SetFocusedFrame, and I'm > > concerned that we'd be losing the source SiteInstance that's passed there if > we > > just use nullptr here. > > Maybe the site instance still needs to be passed along, I'm not quite sure. Note > that this is FrameTree::SetFocusedFrame, not WebContentsImpl. > > For accessibility, each parent needs to correctly show the container frame tree > node of the inner contents as focused so that the > BrowserAccessibility/BrowserAccessibilityManager can walk down and determine > which one is focused. Acknowledged. Thinking about the source SiteInstance some more, it should matter only within the same FrameTree, as it's used to prevent notifying a process that already knows about the focus change. If we're moving to an outer WebContents, that implies a process change, so passing |source| shouldn't be necessary (as it shouldn't match any of the SiteInstances in the outer WC's FrameTree). https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4789: WebContentsImpl* contents_to_focus = inner_contents ? inner_contents : this; On 2017/02/28 20:13:00, avallee wrote: > On 2017/02/23 20:07:04, alexmos wrote: > > I'm a bit confused about this change also. See my earlier question about the > > "SetFocusedFrame(outer_node, nullptr)" above, which I'm guessing is how you > > would get here for the case with a valid inner_contents? Why does this become > > necessary? > > > > Suppose I click on a <webview>. As part of WCI::SetFocusedFrame in the inner > > contents, we call SetAsFocusedWebContentsIfNecessary on the inner contents. > > That now calls the new FocusOuterAttachmentFrameChain, which calls > > SetFocusedFrame(outer_node, nullptr), which ends up here and calls > > SetAsFocusedWebContentsIfNecessary on inner_contents again, which seems > > unnecessary? > > If you clicked the webview before routing the input there would be a call to > WebContentsImpl::FocusOwningWebContents which would focus the inner contents and > then the frame in the outer contents, but that wouldn't call back down here. If > the outer WebContents decided to focus the frame with an inner contents, then we > need to tell the inner contents that it is now active which is why we move > downwards like this. Ack. I was confused that FocusOuterAttachmentFrameChain would call WebContentsImpl::SetFocusedFrame on the outer contents, but it actually calls the FrameTree version, so it doesn't call back down here. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4815: return contents->GetMainFrame(); On 2017/02/28 20:13:00, avallee wrote: > On 2017/02/23 20:07:04, alexmos wrote: > > What makes this necessary? How can we get here in practice? > > On attach or after reaching the end of content, there is no focused frame. > > Should I log a warning here if you think this is unexpected? Not quite sure what you meant by "after reaching the end of content". Does AX call GetFocusedFrameIncludingInnerWebContents on attach when there's no focused frame in the inner contents yet, but the inner contents is already focused in the outer contents? It'd be nice to add rationale for returning the main frame in the comment above the while(). This is probably fine, and I'm not that surprised about returning the main frame (knowing how widely Blink's focusedOrMainFrame() is used), but just want to understand why it's needed here. It seems a bit weird that if there's no focused frame, we return null for outer WC and the main frame for inner WC - would AX break if you returned null here instead? https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:431: auto iter = inner_web_contents_tree_nodes_.find(frame_tree_node_id); nit: might as well just use "node->frame_tree_node_id()" here and remove the extra variable, since it isn't used anywhere else, and should still fit on that line. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4361: // change. In tab navigation can become trapped in the inner contents without By "In tab navigation" do you just mean "Tab navigation"? (i.e. pressing tab/shift-tab?) But then how is it browser-initiated? https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4810: // We cannot return the frame where an inner WebContents is attached, if the I think this is talking about two different things; let's split this sentence into two: one about needing to descend into inner WebContents if the focused frame contains one, another about returning the main frame if the WebContents doesn't have a focused frame. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:881: // List of inner WebContents that we host. Is is indexed by FrameTreeNode nit: s/Is is/It is/ https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1081: // Walks up to the outermost WebContents and focuses the frame tree node where nit: s/frame tree node/FrameTreeNode/. Also, clarify that it will do this for each outer contents in the chain, rather than just the outermost one. E.g., "Walks up the outer WebContents chain and focuses the FrameTreeNode where each inner WebContents is attached."
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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
avallee@chromium.org changed reviewers: - lfg@chromium.org
PTAL. Should be submittable now. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4815: return contents->GetMainFrame(); On 2017/03/03 20:07:57, alexmos wrote: > On 2017/02/28 20:13:00, avallee wrote: > > On 2017/02/23 20:07:04, alexmos wrote: > > > What makes this necessary? How can we get here in practice? > > > > On attach or after reaching the end of content, there is no focused frame. > > > > Should I log a warning here if you think this is unexpected? > > Not quite sure what you meant by "after reaching the end of content". Does AX > call GetFocusedFrameIncludingInnerWebContents on attach when there's no focused > frame in the inner contents yet, but the inner contents is already focused in > the outer contents? It'd be nice to add rationale for returning the main frame > in the comment above the while(). > I think this will get called on attach, but as a result of WebContentsImpl::SetFocusedFrame so there should be one focused already. However, when you tab (shift-tab) with the last (first) focusable element in the document, the FocusController will clear the focused frame. > This is probably fine, and I'm not that surprised about returning the main frame > (knowing how widely Blink's focusedOrMainFrame() is used), but just want to > understand why it's needed here. It seems a bit weird that if there's no > focused frame, we return null for outer WC and the main frame for inner WC - > would AX break if you returned null here instead? As I understand it, it seems that it wants some frame to be focused overall but in a subtree it's fine that there's no focused frame. (See RenderFrameHostImpl::AXContentTreeDataToAXTreeData) https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:431: auto iter = inner_web_contents_tree_nodes_.find(frame_tree_node_id); On 2017/03/03 20:07:57, alexmos wrote: > nit: might as well just use "node->frame_tree_node_id()" here and remove the > extra variable, since it isn't used anywhere else, and should still fit on that > line. This is gone after rebase. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4361: // change. In tab navigation can become trapped in the inner contents without On 2017/03/03 20:07:57, alexmos wrote: > By "In tab navigation" do you just mean "Tab navigation"? (i.e. pressing > tab/shift-tab?) But then how is it browser-initiated? Clarified comment. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4810: // We cannot return the frame where an inner WebContents is attached, if the On 2017/03/03 20:07:57, alexmos wrote: > I think this is talking about two different things; let's split this sentence > into two: one about needing to descend into inner WebContents if the focused > frame contains one, another about returning the main frame if the WebContents > doesn't have a focused frame. rephrased. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:881: // List of inner WebContents that we host. Is is indexed by FrameTreeNode On 2017/03/03 20:07:57, alexmos wrote: > nit: s/Is is/It is/ reverted. https://codereview.chromium.org/2674353003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:1081: // Walks up to the outermost WebContents and focuses the frame tree node where On 2017/03/03 20:07:57, alexmos wrote: > nit: s/frame tree node/FrameTreeNode/. Also, clarify that it will do this for > each outer contents in the chain, rather than just the outermost one. E.g., > "Walks up the outer WebContents chain and focuses the FrameTreeNode where each > inner WebContents is attached." Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
still lgtm, one quick log to remove! https://codereview.chromium.org/2674353003/diff/200001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2674353003/diff/200001/content/renderer/rende... content/renderer/render_frame_impl.cc:2279: LOG(ERROR) << this << "->RenderFrameImpl::OnAdvanceFocus()"; Remove this before submit!
Still LGTM, thanks! https://codereview.chromium.org/2674353003/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4624: // process boundry in focus order, it will not be possible to move across that nit: s/boundry/boundary/ https://codereview.chromium.org/2674353003/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:5079: // frmae embedding this contents. nit: s/frmae/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: 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 wjmaclean@chromium.org, dmazzoni@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2674353003/#ps220001 (title: "Fix typos.")
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": 220001, "attempt_start_ts": 1498082385381230,
"parent_rev": "43e61631023788ec5890948537e81ba6fc612e36", "commit_rev":
"d68cdd1afdff55d24558ef88441d9ac950d46dd4"}
Message was sent while issue was closed.
Description was changed from
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795, 685739
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Enable WebViewAccessiblityTest for OOPIF webview.
+ Enabled WebViewAccessibilityTests for oopif.
+ Hooked up the inner AX tree to point back to the AX tree for the outer web
contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild}
functions fail on the [child->GetParent() == this] check.
BUG=610795, 685739
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2674353003
Cr-Commit-Position: refs/heads/master@{#481326}
Committed:
https://chromium.googlesource.com/chromium/src/+/d68cdd1afdff55d24558ef88441d...
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/d68cdd1afdff55d24558ef88441d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
