[OOPIF] activeElement does not change from focusable element to iframe.
Reported by
cool...@gmail.com,
Aug 13 2017
|
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Steps to reproduce the problem: 1. In the top document, the <button> have focus. 2. User clicks <textarea> in the <iframe>. <textarea> gets focus. In the top document, activeElement == HTMLButtonElement. 3. In the top document, the <button> changes its 'visibility' CSS property to 'hidden'. What is the expected behavior? <textarea> should retain the focus What went wrong? <textarea> lost focus Did this work before? N/A Does this work in other browsers? Yes Chrome version: 60.0.3112.90 (Официальная сборка) (64 бит) (когорта: 60_90_win) Channel: stable OS Version: 6.1 (Windows 7, Windows Server 2008 R2) Flash Version:
,
Aug 14 2017
,
Aug 14 2017
BTW, in step#2 the active element in top document should be "iframe" because its contents is clicked, not "button".
,
Aug 14 2017
See the attached extension. This is definitely OOPIFs, because if you load index.html outside of extension, focus is working as expected. >BTW, in step#2 the active element in top document should be "iframe" because its contents is clicked, not "button". Ooops, you found another issue. :) Check the document.activeElement value in the console.
,
Aug 14 2017
Thank you for providing more feedback. Adding requester "nyerramilli@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 18 2017
Tested on Chrome Stable #60.0.3112.101, Canary #62.0.3189.0 on Windows 7, Mac 10.12.6, Ubuntu 14.04 and able to reproduce the issue. This is a non-regression issue and able to reproduce from M-45 #45.0.2454.85. Marking it as untriaged so that issue gets addressed. Thanks.
,
Aug 21 2017
Adding Internals>Accessibility label, for OOPIF people to take a look at this.
,
Sep 7 2017
avallee@: would you mind triaging this?
,
Sep 7 2017
,
Sep 7 2017
This will be a regression when Site-Isolation is turned on. The actual problem is not so much stealing focus as much as the fact that when clicking the iframe with the button being focused, the iframe does not become focused, instead the button remains the active element until it is hidden. At this point the focused element is cleared and document.activeElement returns <body>.
,
Sep 7 2017
Comment 10: Thanks! Note that we already have launched uses of OOPIFs without full Site Isolation (i.e., web iframes inside extensions, since M56), so this is important already. Can we aim for a fix in M63?
,
Sep 8 2017
@creis: I saw this initially and the main reason I didn't touch this difference was due to the challenge in getting the same order/timing (too early) of the blur event triggering on the previously active element. I'm a bit fuzzy on the exact details I think it was an issue with the when the page was not focused and then was refocused. Teh wrong active element happens here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TreeScope.cpp?type=cs&sq=package:chromium&l=371 after clicking the iframe, element needs to be nullptr. However, when clicking into the oopif, the embedder will get notified of the focus change via the iframe's proxy which calls into FocusDocumentView with not== false (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FocusController.cpp?type=cs&sq=package:chromium&l=785). It looks like a blur event happens here, but the element is not cleared. I tested the example from c#4 loaded as an extension and just as a page and the difference was due to the extension having oopifs (on M-60 stable linux) while the page did not. alexmos@ dcheng@ any tips?
,
Sep 29 2017
,
Sep 29 2017
,
Nov 17 2017
Comment 12: Thanks-- sounds like questions about the replication of focus, and what activeElement should be. (Should it actually be null, though? I'm seeing the activeElement become the iframe in the same-process case, and stay as the button in the OOPIF case, in Windows Canary 64.0.3271.0. I'm guessing we want it to become the iframe?) alexmos@ / dcheng@: Can you take a look at avallee@'s question there? (Also cc'ing lfg@ and kenrb@, in case they have thoughts on it.) Hoping we can get this resolved in the short term, to wrap up high priority input bugs for OOPIFs.
,
Nov 21 2017
I took a look at this. Just for the record, here's my repro: go to http://csreis.github.io/tests/cross-site-iframe.html with --site-per-process, click "go cross-site", then hit tab to focus the "go cross-site" button, then click on the frame. Check main frame's document.activeElement from devtools, and indeed it's <button> instead of <iframe>. I originally added OOPIF support for document.activeElement in https://codereview.chromium.org/1423053002/, and we have a test, DocumentActiveElement, which is supposed to check that the <iframe> element ends up as the activeElement when focusing an OOPIF. It seems that this plumbing gets in trouble when the old document has an existing focused element - as avallee@ pointed out, we aren't clearing the focused element in that old document, but rather only firing blur events on it. Looks like I intentially did this on the CL above -- one of my comments was "I also realized that calling setFocusedElement for this was actually not entirely correct, as it cleared the focused element, which it didn't need to do. The better thing to call was FocusController::focusDocumentView, which fires the same events without clearing the focused element in old focused frame." However, now I can't remember any good reasons for why clearing the focused element wasn't correct. :) In fact, it seems that it is needed, because in the same-process case, we clear it via the following stack, if the old and new focused documents are different in FocusController::SetFocusedElement: #1 0x7f039c00e4d6 blink::Document::SetFocusedElement() #2 0x7f039c00dd69 blink::Document::ClearFocusedElement() #3 0x7f039c912fb1 blink::FocusController::SetFocusedElement() #4 0x7f039c58fdac blink::MouseEventManager::HandleMouseFocus() #5 0x7f039c58309b blink::EventHandler::HandleMousePressEvent() #6 0x7f039c582dde blink::EventHandler::HandleMousePressEvent() #7 0x7f039c927930 blink::PageWidgetEventHandler::HandleMouseDown() Getting an IPC that tells us that a remote frame is now focused implies that we've got a cross-document focus change, and hence we should similarly clear the old focused document's focused element. So, based on this, I think we should try replacing the FocusDocumentView() call that we currently make from RenderFrameProxy::OnSetFocusedFrame(), with "FocusController::SetFocusedElement(nullptr, new_focused_frame)". The latter should dispatch blur events *and* clear the old focused element. Leaving the new focused element as nullptr in that old focused document will also allow its document.activeElement to correctly resolve to the remote focused frame's HTMLFrameOwnerElement. FWIW, I quickly tried this out on a local build, and it seems to resolve the document.activeElement issue, and our existing focus tests all seem to still pass. I'll run it through tryjobs to see if anything else breaks.
,
Nov 21 2017
I had this cl in the works, did this make sense as well?
,
Nov 21 2017
#17: Seems like your CL would change focus behavior for everything, not just OOPIFs. I don't quite understand why it clears the element specifically for ancestors but not other situations -- the stack I mentioned in #16 seems to indicate that we want to clear the old focused document's focused element any time the old document is different from new document, regardless of ancestor chain. Also, I assumed there was a good reason for callers of FocusDocumentView to use that instead of SetFocusedElement, which clears the element together with the blur event. But if the spec and Blink owners are ok with your change, I am too, just seems like a riskier change than what I had in mind. (For reference, avallee@'s CL is https://chromium-review.googlesource.com/721684) The experiment I tried was this: https://chromium-review.googlesource.com/c/chromium/src/+/781424. This alters what we do only for OOPIF scenarios, and tries to match that to same-process behavior. It'd need a bit more work (e.g., rename WebView::FocusDocumentView, which the oopif plumbing uses, to something like SetFocusedFrame, and maybe plumb through notify_embedder to avoid sending extra IPCs back), but it's mostly green, apart from two oopif webview focus tests: WebViewInteractiveTests/WebViewInteractiveTest.KeyboardFocusWindowCycle/1 WebViewInteractiveTests/WebViewInteractiveTest.KeyboardFocusSimple/1 And even those pass on Linux and fail only on Mac and Windows. I'm not sure if that indicates flakiness in the tests vs. some other problem. Maybe you can take a look at those?
,
Nov 22 2017
And just to follow up on #15, I think we'll want to try and fix this before branch cut next Thursday. I'll be OOO until Monday due to Thanksgiving but happy to help with this next week (e.g., write a test or refine the fix) -- just let me know.
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7da6ada25be4cdb742dddbfd6762bd90d872f4cf commit 7da6ada25be4cdb742dddbfd6762bd90d872f4cf Author: Alex Vallée <avallee@chromium.org> Date: Wed Nov 29 20:10:59 2017 Fix incorrect element being focused when changing focused frame. When clicking into an iframe, it was the case the the previously focused element remained focused in the document. This would cause issues for the frame, for example the iframe element would not be the document.activeElement if the previous activeElement was anything but document.body. BUG= 755023 Change-Id: Ibab12e91520e2b9dee5db9eeb0c6c1c61e847928 Reviewed-on: https://chromium-review.googlesource.com/721684 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Alex Vallee <avallee@chromium.org> Cr-Commit-Position: refs/heads/master@{#520215} [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/fast/events/frame-click-clear-focus-expected.txt [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/fast/events/frame-click-clear-focus.html [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/fast/frames/frame-focus-send-blur-expected.txt [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/fast/frames/frame-focus-send-blur.html [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/platform/linux/editing/selection/4975120-expected.png [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/LayoutTests/platform/win/editing/selection/4975120-expected.png [modify] https://crrev.com/7da6ada25be4cdb742dddbfd6762bd90d872f4cf/third_party/WebKit/Source/core/page/FocusController.cpp
,
Nov 29 2017
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be1e40e8555a6489a1bb76bd6c686df0f7fc2f23 commit be1e40e8555a6489a1bb76bd6c686df0f7fc2f23 Author: Alex Vallée <avallee@chromium.org> Date: Thu Nov 30 14:56:39 2017 Add missing Mac test expectation from crrev.com/520215 Initial cl changed focus between iframes and the trybots didn't yell about the missing mac update. BUG= 755023 , 789707 Change-Id: I2bde8238088d3a9c43791c328cde423be810d939 Reviewed-on: https://chromium-review.googlesource.com/798755 Commit-Queue: Alex Vallee <avallee@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#520541} [modify] https://crrev.com/be1e40e8555a6489a1bb76bd6c686df0f7fc2f23/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/be1e40e8555a6489a1bb76bd6c686df0f7fc2f23/third_party/WebKit/LayoutTests/platform/mac/editing/selection/4975120-expected.png
,
Nov 30 2017
,
Jan 17 2018
|
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by kochi@chromium.org
, Aug 14 2017