|
|
DescriptionOOPIF: Enable TabAndMouseFocusNavigation.
Clear the focused element when a remote frame is focused. This can't be done unconditionally without breaking these layout tests.
failures:
fast/frames/frame-focus-send-blur.html
fast/events/frame-click-clear-focus.html
For the OOPIF test to work correctly, we need to tell the FocusController in the focusing renderer to start from the beginning/end of the document when advancing from a parent or sibling frame. If moving from a child keep searching from that child's frame owner element.
It is important to clear the focused element in this document when losing focus to another frame since tab navigation will result in another renderer remotely focusing this document's frame which would dispatch a focus event to the currently focused element.
BUG=702330
Patch Set 1 #Patch Set 2 : log for test failure #Patch Set 3 : Disable on mac and fix layout test. #Patch Set 4 : Exclude ozone since the mouse events don't seem to happen. #
Total comments: 11
Messages
Total messages: 22 (19 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 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 #2 (id:20001) has been deleted
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...
Description was changed from ========== OOPIF: Enable TabAndMouseFocusNavigation. The issue was that the focus was continuing from the previously focused element instead of the first/last focusable element inside the iframed document. BUG=702330 ========== to ========== OOPIF: Enable TabAndMouseFocusNavigation. Clear the focused element when a remote frame is focused. This can't be done unconditionally without breaking these layout tests. failures: fast/frames/frame-focus-send-blur.html fast/events/frame-click-clear-focus.html For the OOPIF test to work correctly, we need to tell the FocusController in the focusing renderer to start from the beginning/end of the document when advancing from a parent or sibling frame. If moving from a child keep searching from that child's frame owner element. It is important to clear the focused element in this document when losing focus to another frame since tab navigation will result in another renderer remotely focusing this document's frame which would dispatch a focus event to the currently focused element. BUG=702330 ==========
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...
avallee@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org
Ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tricky stuff! A few questions below. [+site-isolation-reviews] https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:305: #if (defined(OS_LINUX) && !defined(USE_OZONE)) || defined(OS_WIN) What's wrong with the test on the excluded platforms? Is it possible to get the test working on Mac/ChromeOS? https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:307: // frames in the presence of mouse click initiated focus changes. nit: I'd keep the reference to the bug here. https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:786: if (focusedElement) { I think this needs a comment to explain what's going on. For Daniel's reference, we've discussed this one on https://codereview.chromium.org/2732913005/. I think I also had some questions there about whether this is safe, e.g. I'm still curious whether it's possible to somehow focus a frame but not focus any elements in that frame. That would leave the focused element intact in the old focused frame and break the assumption that this makes. https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:787: if (frame && frame->isRemoteFrame()) Were you referring to this in the description, saying that if you did this always and not just for remote frames, that would break fast/frames/frame-focus-send-blur.html and fast/events/frame-click-clear-focus.html? These two tests currently use data URLs in frames, which stay in-process, but does this mean that if the frames were OOPIFs instead, that this change would be breaking them? https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:942: // first or last focusable element. Why did the old code fail to do this? It seems in this case, |start| would be null, so in advanceFocusInDocumentOrder, |current| would start out as null, and this will not execute anymore due to |initialFocus| being true: if (!current && !initialFocus) current = document->sequentialFocusNavigationStartingPoint(type); Is the issue that sequentialFocusNavigationStartingPoint will return m_focusedElement if it's not null? How will this determine the right starting point now, via ScopedFocusNavigation::createForDocument(*document)? https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:944: to->tree().parent() == from->tree().parent(); Hmm, can we actually get here for a sibling frame? We call RF::advanceFocus in two places, one parent-to-child [1] and the other child-to-parent [2]. It doesn't seem like we could ever go directly from one sibling to another, unless there's now another path to get here other than through advanceFocus. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...
https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:305: #if (defined(OS_LINUX) && !defined(USE_OZONE)) || defined(OS_WIN) On 2017/03/30 17:16:11, alexmos wrote: > What's wrong with the test on the excluded platforms? Is it possible to get the > test working on Mac/ChromeOS? For Mac, I wrote a sample that just prints mousemoves and there's nothing that shows up. On OZONE, the mousemoves seem to show up but then clicks don't happen. https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:307: // frames in the presence of mouse click initiated focus changes. On 2017/03/30 17:16:11, alexmos wrote: > nit: I'd keep the reference to the bug here. Done. https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:786: if (focusedElement) { On 2017/03/30 17:16:11, alexmos wrote: > I think this needs a comment to explain what's going on. > > For Daniel's reference, we've discussed this one on > https://codereview.chromium.org/2732913005/. I think I also had some questions > there about whether this is safe, e.g. I'm still curious whether it's possible > to somehow focus a frame but not focus any elements in that frame. That would > leave the focused element intact in the old focused frame and break the > assumption that this makes. I don't believe that focusing a frame requires a focused element. OTOH: Focusing a widget will focus its main frame if none is focused. Also there is a chain of WebContent -> RenderFrameHost -> RenderFrame -> Document that all provide clearFocusedElement. https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:787: if (frame && frame->isRemoteFrame()) On 2017/03/30 17:16:11, alexmos wrote: > Were you referring to this in the description, saying that if you did this > always and not just for remote frames, that would break > fast/frames/frame-focus-send-blur.html and > fast/events/frame-click-clear-focus.html? These two tests currently use data > URLs in frames, which stay in-process, but does this mean that if the frames > were OOPIFs instead, that this change would be breaking them? I am not sure. They are secondary assertions that have a comment that the result is not important (probably only there for synchronization). The test itself is strange, calling focus() in another iframe does not clear the focused element while clicking the other iframe does. If the test is changed, this code could be simplified and the test would seem more sane. The issue in my test was that in oopif, we were seeing the previously focused element in an iframe have a focus event dispatched to it as part of focusing the frame when tabbing into that frame, whether that element was the correct one to select. Then the appropriate element would get focused. I've tried an alternate implementation but some different tests fail: https://codereview.chromium.org/2796533002/. https://codereview.chromium.org/2780883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:942: // first or last focusable element. On 2017/03/30 17:16:11, alexmos wrote: > Why did the old code fail to do this? It seems in this case, |start| would be > null, so in advanceFocusInDocumentOrder, |current| would start out as null, and > this will not execute anymore due to |initialFocus| being true: > > if (!current && !initialFocus) > current = document->sequentialFocusNavigationStartingPoint(type); > > Is the issue that sequentialFocusNavigationStartingPoint will return > m_focusedElement if it's not null? How will this determine the right starting > point now, via ScopedFocusNavigation::createForDocument(*document)? |start| is null but sequentialFocusNavigationStartingPoint starts from the currently focused element, otherwise the m_sequentialFocusStartingPoint [1]. We don't want that, instjead we want the starting point to be the initial focus point, so we want to skip this check and then ScopedFocusNavigation::createForDocument(Document*) instead of the ScopedFocusNavigation::createFor(Node*). The other problem is the extra event dispatching. In non-oopif, we will get a single focus event on the new focused element. With the current oopif implementation, the renderer that processes the advance focus will (below around line 1024) setFocus on the remote frame and then advance focus. This has the effect of refocusing the previous element [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... |