Bug 86969

Created attachment 142917 [details] Patch

Comment on attachment 142917 [details] Patch Attachment 142917 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12733443 New failing tests: fast/dom/HTMLDocument/hasFocus.html

Created attachment 142918 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 142917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142917&action=review I wonder if all ports want this change. Do you think we should have a WebCore::Setting to enable this new behavior? It might be worth asking on webkit-dev to see if folks would like such a setting. > Source/WebCore/ChangeLog:9 > + Disallow window.blur altogether, and only allow window.focus to be > + invoked from the context that created this very window. This patch doesn't seem to change blur at all... > Source/WebCore/ChangeLog:11 > + No new tests yet. I guess this will break some existing tests as well (OOPS!) Looks like it only broke one test. > Source/WebCore/page/Chrome.cpp:170 > +bool Chrome::canSetFocus() const > +{ > + return m_client->canSetFocus(); > +} There's no need for this sort of do-nothing function. The callers can just talk to the client directly. > Source/WebCore/page/ChromeClient.h:94 > + virtual bool canSetFocus() const { return false; } Is the plan to override this function at some point? I'm not sure I understand the role it plays in this patch.

Eric suggests that another way to deal with popunders is to close the "popup" if we detect that it has become a popunder.

(In reply to comment #4) > (From update of attachment 142917 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142917&action=review > > I wonder if all ports want this change. Do you think we should have a WebCore::Setting to enable this new behavior? It might be worth asking on webkit-dev to see if folks would like such a setting. > I asked here: http://lists.macosforge.org/pipermail/webkit-dev/2012-April/020172.html and but I can ask more explicitly again > > Source/WebCore/ChangeLog:9 > > + Disallow window.blur altogether, and only allow window.focus to be > > + invoked from the context that created this very window. > > This patch doesn't seem to change blur at all... Since canSetFocus() defaults to false, unfocus is never invoked. > > > Source/WebCore/ChangeLog:11 > > + No new tests yet. I guess this will break some existing tests as well (OOPS!) > > Looks like it only broke one test. Test coverage seems overrated :-/ > > > Source/WebCore/page/Chrome.cpp:170 > > +bool Chrome::canSetFocus() const > > +{ > > + return m_client->canSetFocus(); > > +} > > There's no need for this sort of do-nothing function. The callers can just talk to the client directly. > > > Source/WebCore/page/ChromeClient.h:94 > > + virtual bool canSetFocus() const { return false; } > > Is the plan to override this function at some point? I'm not sure I understand the role it plays in this patch. The idea was that one might want to override this function for e.g. app extents in chromium, or if you embed a webview somewhere that expects this to work "the old way"

> The idea was that one might want to override this function for e.g. app extents in chromium, or if you embed a webview somewhere that expects this to work "the old way" I would leave the function out until we actually want to do that.

Comment on attachment 142917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142917&action=review > Source/WebCore/page/DOMWindow.cpp:942 > - page->chrome()->unfocus(); > + if (page->chrome()->canSetFocus()) > + page->chrome()->unfocus(); I see. I missed this part of the change.

(In reply to comment #5) > Eric suggests that another way to deal with popunders is to close the "popup" if we detect that it has become a popunder. I'd rather match Firefox here than to fragment the possible UA behavior further

Created attachment 142972 [details] Patch

Comment on attachment 142972 [details] Patch Attachment 142972 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12730609 New failing tests: fast/events/change-frame-focus.html

Created attachment 143012 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 143038 [details] Patch

Comment on attachment 143038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143038&action=review This looks fine, but please wait a bit before landing this patch in case folks on webkit-dev have comments. > LayoutTests/fast/dom/HTMLDocument/hasFocus.html:32 > - checkFocus(false, false); > + checkFocus(true, false); I'd add a comment about how window.blur() doesn't do anything anymore because of popupunders. Otherwise, someone reading this test might think this is pretty odd. > LayoutTests/fast/dom/Window/mozilla-focus-blur.html:19 > + var log = document.getElementById('log'); > + var escaped = message.replace(/</g, '&lt;'); > + if (isOk) > + log.innerHTML += 'PASS: ' + escaped + '<br>'; > + else > + log.innerHTML += 'FAIL: ' + escaped + '<br>'; Why not just use document.createTextNode and appendChild() ? > LayoutTests/fast/dom/Window/mozilla-focus-blur.html:22 > +function focusShouldNotChange(aAction, nextTest) { Is there a better name we can use than aAction? That looks like a name Mozilla would use. > LayoutTests/fast/dom/Window/mozilla-focus-blur.html:77 > + focusShouldNotChange2('data:text/html,\<script>opener.focus();opener.postMessage("", "*");\<\/script>', test4); \< <-- The \ isn't needed.

Comment on attachment 143038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143038&action=review > Source/WebCore/page/DOMWindow.cpp:915 > + if (activeDocument && opener() && activeDocument->domWindow() == opener()) So, I don't think the if (activeDocument) is needed in this clause (since this is already wrapped in if(context). Also, I'm concerned that this change will prevent gmail chat notifications from focusing the gmail window. Currently, the code does this: notification.onclick = function() { // Called in the context of the parent window. // User clicked the notification, so bring the gmail window to the front. window.focus(); } This change basically breaks notifications because notifications mostly want to be able to focus the parent window when they are clicked on.

(In reply to comment #15) > (From update of attachment 143038 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143038&action=review > > > Source/WebCore/page/DOMWindow.cpp:915 > > + if (activeDocument && opener() && activeDocument->domWindow() == opener()) > > So, I don't think the if (activeDocument) is needed in this clause (since this is already wrapped in if(context). True. I copied this from DOMWindow::close, where this also seems to be superfluous. > > Also, I'm concerned that this change will prevent gmail chat notifications from focusing the gmail window. Currently, the code does this: > > notification.onclick = function() { > // Called in the context of the parent window. > // User clicked the notification, so bring the gmail window to the front. > window.focus(); > } > > This change basically breaks notifications because notifications mostly want to be able to focus the parent window when they are clicked on. We could introduce some ScopedAllowFocus class as Darin suggested earlier to enable this use case.

Interesting idea, although adding ScopedAllowFocus would still allow apps to display popunders in their notification onclick handlers. But that's probably not a huge use case. In any case, I would encourage you not to land this change until we add some kind of accommodation for the notifications onclick API.

Created attachment 144221 [details] Patch

Created attachment 144222 [details] Patch

I believe I addressed all comments I added a new setting (windowFocusRestricted) that defaults to true. It's also accessible via window.internals.settings for tests. I also added a class WindowFocusAllowedIndicator to temporarily allow window.focus() and use it for notifications (also added a test for it)

Comment on attachment 144222 [details] Patch Attachment 144222 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12794929

Created attachment 144224 [details] Patch

Comment on attachment 144224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144224&action=review > Source/WebCore/GNUmakefile.list.am:3081 > + Source/WebCore/page/WindowFocusAllowedIndicator.cpp \ > + Source/WebCore/page/WindowFocusAllowedIndicator.h \ Bad indent. > Source/WebCore/notifications/Notification.cpp:226 > + // During clicks on notifications, the page is allowed to focus itself. This comment is redundant with the code below. > Source/WebCore/page/DOMWindow.cpp:913 > + Settings* settings = m_frame->settings(); > + bool allowFocus = WindowFocusAllowedIndicator::windowFocusAllowed() || (settings && !settings->windowFocusRestricted()); Note: settings will always be non-null here because page is non-null. > Source/WebCore/page/DOMWindow.cpp:917 > + // Only allow our opener to focus us. This comment is redundant with the code below. > Source/WebCore/page/WindowFocusAllowedIndicator.h:35 > +// Lifts restrictions on window.focus() and window.blur() as imposed by WebCore > +// during its lifetime. window.blur doesn't seem to interact with WindowFocusAllowedIndicator. I'd probably just remove this comment because it's likely to get out of date. The real purpose of this class is easy to determine by grepping the codebase. > Source/WebKit/chromium/src/WebNotification.cpp:144 > // Make sure clicks on notifications are treated as user gestures. > UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture); > + // During clicks on notifications, the page is allowed to focus itself. > + WindowFocusAllowedIndicator windowFocusAllowed; > dispatchEvent(eventNames().clickEvent); I would remove both of these comments. They're just noise.

Created attachment 144750 [details] patch for landing

Created attachment 144763 [details] patch for landing (rebased)

Comment on attachment 144763 [details] patch for landing (rebased) Clearing flags on attachment: 144763 Committed r118916: <http://trac.webkit.org/changeset/118916>

All reviewed patches have been landed. Closing bug.

(In reply to comment #27) > All reviewed patches have been landed. Closing bug. fast/dom/Window/mozilla-focus-blur.html has been consistently failing in both Mac and Gtk+ ports since it landed

(In reply to comment #28) > (In reply to comment #27) > > All reviewed patches have been landed. Closing bug. > > fast/dom/Window/mozilla-focus-blur.html has been consistently failing in both Mac and Gtk+ ports since it landed Also Qt

Looking

(In reply to comment #30) > Looking I landed failing mac results in http://trac.webkit.org/changeset/118953 to help the bots out, please revert when you fix.

(In reply to comment #30) > Looking Skipped on Qt after r118916: fast/dom/Window/mozilla-focus-blur.html fast/notifications/notifications-click-event-focus.html Please unskip these tests when its fixed.

Skipped for GTK too, and added a new bug to keep track of this: https://bugs.webkit.org/show_bug.cgi?id=87951

I modified DRT for mac so that it displays the window on screen (instead of off-screen). It seems that DRT and WKTR both don't support changing the focus between different windows (i.e. a simple test window.open('about:blank).focus() won't focus the newly created window (even without this CL). This is a bit unfortunate, as it makes testing this change hardly possible on those platforms :-/