Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(285)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2780883003: OOPIF: Enable TabAndMouseFocusNavigation. (Closed)

Created:
3 years, 8 months ago by avallee
Modified:
3 years, 5 months ago
Reviewers:
alexmos, dcheng
CC:
chromium-reviews, blink-reviews, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -6 lines) Patch
M chrome/browser/site_per_process_interactive_browsertest.cc View 1 2 3 3 chunks +3 lines, -3 lines 4 comments Download
M third_party/WebKit/Source/core/page/FocusController.cpp View 1 2 2 chunks +12 lines, -3 lines 7 comments Download

Messages

Total messages: 22 (19 generated)
avallee
Ptal
3 years, 8 months ago (2017-03-29 23:49:29 UTC) #18
alexmos
Tricky stuff! A few questions below. [+site-isolation-reviews] https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per_process_interactive_browsertest.cc File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2780883003/diff/80001/chrome/browser/site_per_process_interactive_browsertest.cc#newcode305 chrome/browser/site_per_process_interactive_browsertest.cc:305: #if (defined(OS_LINUX) ...
3 years, 8 months ago (2017-03-30 17:16:12 UTC) #21
avallee
3 years, 8 months ago (2017-04-02 02:27:30 UTC) #22 
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...

Powered by Google App Engine
This is Rietveld 408576698