|
|
Description<webview>: Add test to confirm the TAB key will escape the view.
There is a corner case where the embedder may think the element directly
following the webview has focus prevents focusing moving out of the
webview with the tab key.
This problem is a combination of the embedder not knowing the webview is
focused and some early out logic when landing on the next element
because it is already focused.
BUG=674219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2732913005
Cr-Commit-Position: refs/heads/master@{#470855}
Committed: https://chromium.googlesource.com/chromium/src/+/9edd151ead7afa9dc3f8cb3f4620adf949eb119c
Patch Set 1 #Patch Set 2 : Fix the focus switch in blink. #Patch Set 3 : Fix clearFocusedElement, only if focusing a child frame. #
Total comments: 18
Patch Set 4 : rebase (focus controller change in ToT) #
Messages
Total messages: 31 (22 generated)
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 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_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...
Description was changed from ========== <webview>: Add test to confirm the TAB key will escape the view. There is a corner case where the embedder may think the element directly following the webview has focus prevents focusing moving out of the webview with the tab key. This problem is a combination of the embedder not knowing the webview is focused and some early out logic when landing on the next element because it is already focused. BUG=694682 ========== to ========== <webview>: Add test to confirm the TAB key will escape the view. There is a corner case where the embedder may think the element directly following the webview has focus prevents focusing moving out of the webview with the tab key. This problem is a combination of the embedder not knowing the webview is focused and some early out logic when landing on the next element because it is already focused. BUG=674219 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.
avallee@chromium.org changed reviewers: + alexmos@chromium.org
PTAL: This seems to pass all blink tests and fixes the issue where focusing the webview does not unfocus other elements in the embedder. wjm,lfg: FYI
https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:673: IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusTakeFocus) { Since this is also a problem for OOPIFs without <webview>, it'd be great to also have a pure OOPIF test for this, i.e., something along the lines of SitePerProcessInteractiveBrowserTest.SequentialFocusNavigation. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:789: newFrame = newFrame->client()->parent()) { Why do we need to walk up the ancestor chain? Is this for the case that intermediate frames don't have any focusable elements? https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:795: if (focusedFrameIsAncestor) What about the reverse case, where an <input> that has focus is before the OOPIF, then the OOPIF is clicked, and then you shift-tab upwards? Does that case also work? https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:796: document->clearFocusedElement(); Will |frame| always be a RemoteFrame when this is needed? Also, this could use a comment with an explanation. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:798: dispatchBlurEvent(*document, *focusedElement); Just to check, does clearFocusedElement also dispatch this blur event?
https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:673: IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusTakeFocus) { On 2017/03/10 07:08:33, alexmos wrote: > Since this is also a problem for OOPIFs without <webview>, it'd be great to also > have a pure OOPIF test for this, i.e., something along the lines of > SitePerProcessInteractiveBrowserTest.SequentialFocusNavigation. Let me add this in then. Just to be clear, I think you're saying similar to the SequentialFocusNavigation test, except that you click into the oopif and try to navigate out (and maybe also click outside and try to navigate in). https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:789: newFrame = newFrame->client()->parent()) { On 2017/03/10 07:08:33, alexmos wrote: > Why do we need to walk up the ancestor chain? Is this for the case that > intermediate frames don't have any focusable elements? I'm not sure how exiting a frame works. In the top > iframe > webview structure, I'm not sure if there were no focusable elements in the iframe if the return to top from webview is done in 1 or 2 steps. If it's done in 2 steps with focus momentarily passing through the iframe, then this should not be needed. If however, the focus passes straight through, it's possible that the top frame now tries to advance and has an element focused. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:795: if (focusedFrameIsAncestor) On 2017/03/10 07:08:33, alexmos wrote: > What about the reverse case, where an <input> that has focus is before the > OOPIF, then the OOPIF is clicked, and then you shift-tab upwards? Does that > case also work? This was doing the wrong thing when the browser notified you that the guest has become focused. Tab/Shift-tab would be sent to a renderer who might decide to initiate a focus change into a remote parent or child frame. When this renderer initiate the focus change, it will be in a consistent state. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:796: document->clearFocusedElement(); On 2017/03/10 07:08:33, alexmos wrote: > Will |frame| always be a RemoteFrame when this is needed? Also, this could use > a comment with an explanation. I think the issue would only happen with remote frames. On a local frame, the change happens within the renderer and things are set correctly at that point. Here we were reacting to a remote renderer changing focus. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:798: dispatchBlurEvent(*document, *focusedElement); On 2017/03/10 07:08:33, alexmos wrote: > Just to check, does clearFocusedElement also dispatch this blur event? Yes, it calls setFocusedElement(nullptr, /*some other stuff*/) and then dispatched on the oldFocusedElement: http://google3/third_party/chromium_headless/blink/Source/core/dom/Document.c...
+site-isolation-reviews, since this concerns OOPIF focus as well. Sorry for the delay! Took me a while to think through the issues here, but some thoughts below. https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:673: IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusTakeFocus) { On 2017/03/13 19:18:10, avallee wrote: > On 2017/03/10 07:08:33, alexmos wrote: > > Since this is also a problem for OOPIFs without <webview>, it'd be great to > also > > have a pure OOPIF test for this, i.e., something along the lines of > > SitePerProcessInteractiveBrowserTest.SequentialFocusNavigation. > > Let me add this in then. Just to be clear, I think you're saying similar to the > SequentialFocusNavigation test, except that you click into the oopif and try to > navigate out (and maybe also click outside and try to navigate in). Yes, exactly. I'm just thinking of following the repro steps in 674219. Thanks! https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:787: bool focusedFrameIsAncestor = false; I think this is really newFocusedFrameIsDescendant, but it doesn't matter -- see below. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:789: newFrame = newFrame->client()->parent()) { On 2017/03/13 19:18:10, avallee wrote: > On 2017/03/10 07:08:33, alexmos wrote: > > Why do we need to walk up the ancestor chain? Is this for the case that > > intermediate frames don't have any focusable elements? > > I'm not sure how exiting a frame works. In the top > iframe > webview structure, > I'm not sure if there were no focusable elements in the iframe if the return to > top from webview is done in 1 or 2 steps. If it's done in 2 steps with focus > momentarily passing through the iframe, then this should not be needed. If > however, the focus passes straight through, it's possible that the top frame now > tries to advance and has an element focused. I was really confused here and what this has to do with exiting a frame, but I think I understand things better now after following some code around and patching in your CL. I mostly thought about this with just OOPIFs and no webviews, since that's easier and the fix is the same for both. So, the intent of this code is really to clear the focused element when we learn that a remote frame has become focused. I.e., in the bug repro, we have: <iframe A> <iframe B> <input 2> </iframe> <input 1> <-- focused initially </iframe> The <input 1> is focused initially, so frame A is the focused frame. You then click on <input 2>. At that point, the B renderer makes frame B the focused frame and forwards the focused frame change to renderer A, which will end up calling focusDocumentView() on the RemoteFrame for B. At this point, you get to this code and want to clear focused element (<input>). A few concerns about this approach: 1. I'm not convinced this should be gated on the ancestor walk. Consider this example: <iframe A> <input 1> <-- click this second <iframe B> <input 2> <-- click this first </iframe> </iframe> Then press tab. Wouldn't that have the same problem unless you clear the focused element (<input 2>) in frame B? The current code won't do it, since it'll get here in renderer B, with A being the new focused frame, i.e., B will be the descendant not an ancestor of the new focused frame. In practice, I tried this out on http://csreis.github.io/tests/cross-site-iframe-simple.html with --site-per-process: a. in devtools, document.querySelector("h1").appendChild(document.createElement("input")) b. click first <input> in iframe c. click the main frame's <input> created in a) d. press tab Focus should've switch to the first <input> in the iframe, but instead it shifts to the *second* input there. I don't quite understand why it does that, and whether that's due to not calling clearFocusedElement() in B when we clicked on the <input 1> in A, or a different issue. Thoughts? (It'd be good to add a test for this case as well.) 2. I was thinking if we should instead just call clearFocusedElement when |frame| is a remote frame. That should fix 1 and any similar cases (sibling iframes?). But I can't convince myself that it's safe. Thinking more carefully about how this works without any OOPIFs today, it seems that normally clearFocusedElement is called as part of FocusController::setFocusedElement if the old focused element was in a different frame, as part of mouse click dispatch (EventHandler::handleMousePressEvent -> MouseEventManager::handleMouseFocus -> FocusController::setFocusedElement -> Document::clearFocusedElement). However, setFocusedElement is not called unconditionally in handleMouseFocus; there are a few WebInputEventResult::NotHandled early returns that I don't fully understand (e.g., for clicking on scrollbar, some drag stuff). What I'm wondering is whether it's possible to click on a frame and focus that frame, but follow one of those early returns and not focus any elements in that frame (which would leave the focused element intact in the old focused frame). If not, maybe we can get away with clearFocusedElement when |frame| is a remote frame. If yes, we may need to send a separate IPC to clear the focused element in the old focused frame only when needed. Maybe you could try seeing if any of those early returns matter in practice? FWIW, although usually FocusController::setFocusedFrame is called as part of FocusController::setFocusedElement, it's also called again via MouseEventManager::handleMousePressEvent -> MouseEventManager::focusDocumentView -> FocusController::focusDocumentView -> setFocusedFrame (which is usually a noop), but could matter in case we end up not calling setFocusedElement. Regarding your question on how tab traversal exits a frame, I think it's a one-step process. If you clicked on a remote grandchild frame nested in a middle frame with no focusable elements, and then pressing tab to get out to the root frame, advanceFocusInDocumentOrder and findFocusableElementAcrossFocusScopes will look for elements to focus in all possible local frames, including ancestor frames if they exist, in one step before handing the search over to the remote parent of the local root. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:795: if (focusedFrameIsAncestor) On 2017/03/13 19:18:10, avallee wrote: > On 2017/03/10 07:08:33, alexmos wrote: > > What about the reverse case, where an <input> that has focus is before the > > OOPIF, then the OOPIF is clicked, and then you shift-tab upwards? Does that > > case also work? > > This was doing the wrong thing when the browser notified you that the guest has > become focused. Tab/Shift-tab would be sent to a renderer who might decide to > initiate a focus change into a remote parent or child frame. When this renderer > initiate the focus change, it will be in a consistent state. I'm not sure I followed your explanation, but I think the case I asked about works. :) It doesn't matter where the previously focused element is, we'll clear it regardless when we click on a descendant of a currently focused frame, whether it's an OOPIF or webview. So whether we get to the element via tab or shift-tab later doesn't matter. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:796: document->clearFocusedElement(); On 2017/03/13 19:18:10, avallee wrote: > On 2017/03/10 07:08:33, alexmos wrote: > > Will |frame| always be a RemoteFrame when this is needed? Also, this could > use > > a comment with an explanation. > > I think the issue would only happen with remote frames. On a local frame, the > change happens within the renderer and things are set correctly at that point. > > Here we were reacting to a remote renderer changing focus. See the big comment above regarding this. https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:798: dispatchBlurEvent(*document, *focusedElement); On 2017/03/13 19:18:10, avallee wrote: > On 2017/03/10 07:08:33, alexmos wrote: > > Just to check, does clearFocusedElement also dispatch this blur event? > > Yes, it calls setFocusedElement(nullptr, /*some other stuff*/) and then > dispatched on the oldFocusedElement: > http://google3/third_party/chromium_headless/blink/Source/core/dom/Document.c... Acknowledged.
https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2732913005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:789: newFrame = newFrame->client()->parent()) { On 2017/03/16 17:37:11, alexmos wrote: > On 2017/03/13 19:18:10, avallee wrote: > > On 2017/03/10 07:08:33, alexmos wrote: > > > Why do we need to walk up the ancestor chain? Is this for the case that > > > intermediate frames don't have any focusable elements? > > > > I'm not sure how exiting a frame works. In the top > iframe > webview > structure, > > I'm not sure if there were no focusable elements in the iframe if the return > to > > top from webview is done in 1 or 2 steps. If it's done in 2 steps with focus > > momentarily passing through the iframe, then this should not be needed. If > > however, the focus passes straight through, it's possible that the top frame > now > > tries to advance and has an element focused. > > I was really confused here and what this has to do with exiting a frame, but I > think I understand things better now after following some code around and > patching in your CL. I mostly thought about this with just OOPIFs and no > webviews, since that's easier and the fix is the same for both. > > So, the intent of this code is really to clear the focused element when we learn > that a remote frame has become focused. I.e., in the bug repro, we have: > <iframe A> > <iframe B> > <input 2> > </iframe> > <input 1> <-- focused initially > </iframe> > > The <input 1> is focused initially, so frame A is the focused frame. You then > click on <input 2>. At that point, the B renderer makes frame B the focused > frame and forwards the focused frame change to renderer A, which will end up > calling focusDocumentView() on the RemoteFrame for B. At this point, you get to > this code and want to clear focused element (<input>). > > A few concerns about this approach: > > 1. I'm not convinced this should be gated on the ancestor walk. Consider this > example: > > <iframe A> > <input 1> <-- click this second > <iframe B> > <input 2> <-- click this first > </iframe> > </iframe> > > Then press tab. Wouldn't that have the same problem unless you clear the > focused element (<input 2>) in frame B? The current code won't do it, since > it'll get here in renderer B, with A being the new focused frame, i.e., B will > be the descendant not an ancestor of the new focused frame. > > In practice, I tried this out on > http://csreis.github.io/tests/cross-site-iframe-simple.html with > --site-per-process: > a. in devtools, > document.querySelector("h1").appendChild(document.createElement("input")) > b. click first <input> in iframe > c. click the main frame's <input> created in a) > d. press tab > Focus should've switch to the first <input> in the iframe, but instead it > shifts to the *second* input there. I don't quite understand why it does that, > and whether that's due to not calling clearFocusedElement() in B when we clicked > on the <input 1> in A, or a different issue. Thoughts? > > (It'd be good to add a test for this case as well.) > > 2. I was thinking if we should instead just call clearFocusedElement when > |frame| is a remote frame. That should fix 1 and any similar cases (sibling > iframes?). But I can't convince myself that it's safe. > > Thinking more carefully about how this works without any OOPIFs today, it seems > that normally clearFocusedElement is called as part of > FocusController::setFocusedElement if the old focused element was in a different > frame, as part of mouse click dispatch (EventHandler::handleMousePressEvent -> > MouseEventManager::handleMouseFocus -> FocusController::setFocusedElement -> > Document::clearFocusedElement). However, setFocusedElement is not called > unconditionally in handleMouseFocus; there are a few > WebInputEventResult::NotHandled early returns that I don't fully understand > (e.g., for clicking on scrollbar, some drag stuff). > > What I'm wondering is whether it's possible to click on a frame and focus that > frame, but follow one of those early returns and not focus any elements in that > frame (which would leave the focused element intact in the old focused frame). > If not, maybe we can get away with clearFocusedElement when |frame| is a remote > frame. If yes, we may need to send a separate IPC to clear the focused element > in the old focused frame only when needed. Maybe you could try seeing if any of > those early returns matter in practice? > > FWIW, although usually FocusController::setFocusedFrame is called as part of > FocusController::setFocusedElement, it's also called again via > MouseEventManager::handleMousePressEvent -> MouseEventManager::focusDocumentView > -> FocusController::focusDocumentView -> setFocusedFrame (which is usually a > noop), but could matter in case we end up not calling setFocusedElement. > > Regarding your question on how tab traversal exits a frame, I think it's a > one-step process. If you clicked on a remote grandchild frame nested in a > middle frame with no focusable elements, and then pressing tab to get out to the > root frame, advanceFocusInDocumentOrder and > findFocusableElementAcrossFocusScopes will look for elements to focus in all > possible local frames, including ancestor frames if they exist, in one step > before handing the search over to the remote parent of the local root. I'll also note that at some point, when replicating the new focused frame I tried using setFocusedElement(nullptr, newFocusedFrame) instead of focusDocumentView, which would've avoided this bug since it clears the old focused element. I can't remember all the details, but I think I ran into issues with that, possibly because of window.focus called on remote frames? See https://codereview.chromium.org/1423053002/#msg4 and PS1->PS2. (Just mentioning this as that's another alternative for a fix.)
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: + wjmaclean@chromium.org
wjmaclean: PTAL. https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2732913005/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:673: IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, Focus_FocusTakeFocus) { On 2017/03/16 17:37:11, alexmos wrote: > On 2017/03/13 19:18:10, avallee wrote: > > On 2017/03/10 07:08:33, alexmos wrote: > > > Since this is also a problem for OOPIFs without <webview>, it'd be great to > > also > > > have a pure OOPIF test for this, i.e., something along the lines of > > > SitePerProcessInteractiveBrowserTest.SequentialFocusNavigation. > > > > Let me add this in then. Just to be clear, I think you're saying similar to > the > > SequentialFocusNavigation test, except that you click into the oopif and try > to > > navigate out (and maybe also click outside and try to navigate in). > > Yes, exactly. I'm just thinking of following the repro steps in 674219. > Thanks! Test created and enabled (flaky but quite rarely) with https://crbug.com/713977
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by avallee@chromium.org
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": 60001, "attempt_start_ts": 1494485835694340, "parent_rev": "d864c36cdca36cf097210057df806da44d98291a", "commit_rev": "9edd151ead7afa9dc3f8cb3f4620adf949eb119c"}
Message was sent while issue was closed.
Description was changed from ========== <webview>: Add test to confirm the TAB key will escape the view. There is a corner case where the embedder may think the element directly following the webview has focus prevents focusing moving out of the webview with the tab key. This problem is a combination of the embedder not knowing the webview is focused and some early out logic when landing on the next element because it is already focused. BUG=674219 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== <webview>: Add test to confirm the TAB key will escape the view. There is a corner case where the embedder may think the element directly following the webview has focus prevents focusing moving out of the webview with the tab key. This problem is a combination of the embedder not knowing the webview is focused and some early out logic when landing on the next element because it is already focused. BUG=674219 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2732913005 Cr-Commit-Position: refs/heads/master@{#470855} Committed: https://chromium.googlesource.com/chromium/src/+/9edd151ead7afa9dc3f8cb3f4620... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9edd151ead7afa9dc3f8cb3f4620... |