This bug can be reproduced with the following way. (1) open the attached file. (2) Middle click the named anchor to open a document (the document is opened in a background tab.) (3) show the opened document. (4) show the original document and middle click the named anchor again. (5) the document opened in (2) is foreground now.
Created attachment 94168 [details] HTML file containing named target.
I cannot reproduce this in Safari 5.0.5 on Mac OS X . What browser have you been testing with?
(In reply to comment #2) > I cannot reproduce this in Safari 5.0.5 on Mac OS X . What browser have you been testing with? I have tested in Safari 5.0.4 on Mac OS X (Snow leopard). Let me have time to check it in Safari 5.0.5 now.
Created attachment 94182 [details] Another file.
The newly attached file can reproduce it. (When middle clicking, 404 is displayed but please ignore 404.) Since the difference is the domain, when opening the same domain file this bug may occur.
(In reply to comment #5) > The newly attached file can reproduce it. (When middle clicking, 404 is displayed but please ignore 404.) > > Since the difference is the domain, when opening the same domain file this bug may occur. This is confirmed in Safari 5.0.5.
Created attachment 94183 [details] patch to fix the problem I'd like to write a test, but I have not written it well, so I upload only this patch for now.
Comment on attachment 94183 [details] patch to fix the problem View in context: https://bugs.webkit.org/attachment.cgi?id=94183&action=review > Source/WebCore/loader/FrameLoader.cpp:1379 > + && !openedBackgroundTab() && !targetFrame->loader()->openedBackgroundTab()) { Why do you need to test whether this frame *and* the target frame haven't opened a background tab? Can't the canonical "background"-ness be stored in one place? > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:853 > + m_webFrame->frame()->loader()->setOpenedBackgroundTab(true); This should be indented two more spaces. > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:971 > + if (navigationPolicy == WebNavigationPolicyNewBackgroundTab) Why does this need to happen in two places? > Source/WebKit/chromium/src/FrameLoaderClientImpl.h:253 > + bool m_openedBackgroundTab; Who uses this one?
(In reply to comment #8) > (From update of attachment 94183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94183&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1379 > > + && !openedBackgroundTab() && !targetFrame->loader()->openedBackgroundTab()) { > > Why do you need to test whether this frame *and* the target frame haven't opened a background tab? Can't the canonical "background"-ness be stored in one place? Usually the FrameLoader of the source Frame opens a new tab when middle click. however, when there is a tab having target name already, the FrameLoader of that tab opens a new tab. I wanted to unify it, but it became very messy. > > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:971 > > + if (navigationPolicy == WebNavigationPolicyNewBackgroundTab) > > Why does this need to happen in two places? Because creating new Tab and changing Tab content are different method. > > > Source/WebKit/chromium/src/FrameLoaderClientImpl.h:253 > > + bool m_openedBackgroundTab; > > Who uses this one? Sorry, this is my mistake.
> The newly attached file can reproduce it. (When middle clicking, 404 is displayed but please ignore 404.) I still cannot reproduce this - perhaps because I don't have a middle mouse button, and tested Cmd-clicking. Weird. Please be sure to mark the patch for review (review? flag) when it's ready, so that it doesn't get overlooked.
Ah, (In reply to comment #10) > I still cannot reproduce this - perhaps because I don't have a middle mouse button, and tested Cmd-clicking. Weird. I could reproduce it. You should display the opened file once ((3) is so). Without this step, Safari will work as expected. > Please be sure to mark the patch for review (review? flag) when it's ready, so that it doesn't get overlooked. I would like to have a test, but I don't know how to do it...
I observe the bug in both of Kawanaka's examples in Google Chrome 12 but not in Safari 5.1 (Windows). Here are related bugs in the Chrome bug tracker http://code.google.com/p/chromium/issues/detail?id=62319 http://code.google.com/p/chromium/issues/detail?id=84514
Hi, I confirmed that my example still works in Lion's Safari in another situation. 1) Open Another file. (attached one) 2) click the link (this will create another window, and NotFound page is displayed.) 3) back to the (1) page 4) the Cmd+click The last Cmd+click should open a tab in a background, however this focuses the window created in (2). (In reply to comment #12) > I observe the bug in both of Kawanaka's examples in Google Chrome 12 but not in Safari 5.1 (Windows). > > Here are related bugs in the Chrome bug tracker http://code.google.com/p/chromium/issues/detail?id=62319 http://code.google.com/p/chromium/issues/detail?id=84514
I made this issue chromium-specific. The Safari's problem is moved to https://bugs.webkit.org/show_bug.cgi?id=65454
Created attachment 102492 [details] Patch
Comment on attachment 102492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102492&action=review > Source/WebCore/loader/FrameLoader.h:431 > + bool m_openedBackgroundTab; How about to mirror navigation policy enum here instead of using enum? See Source/WebKit/chromium/src/AssertMatchingEnums.cpp to see similar patterns. > Source/WebKit/chromium/src/FrameLoaderClientImpl.h:255 > + bool m_openedBackgroundTab; It looks we don't need this.
Created attachment 103016 [details] Patch
Comment on attachment 103016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103016&action=review > Source/WebCore/ChangeLog:1 > +2011-08-04 Shinya Kawanaka <shinyak@google.com> You should add NavigationPolicy.h to build configurations. For example WebCore.gypi for Chromium. > Source/WebCore/loader/NavigationPolicy.h:1 > +/* Why not use FrameLoaderTypes.h ? And I noticed it might be possible to extend PolicyAction to support this. But I'm not sure. > Source/WebKit/chromium/public/WebNavigationPolicy.h:37 > + WebNavigationPolicyInvalid = -1, Why -1?
Created attachment 103215 [details] Patch
Comment on attachment 103215 [details] Patch Attachment 103215 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9215177
Created attachment 103222 [details] Patch
I often encounter this bug and every time I encounter this bug I am disappointed that this patch has not been reviewed yet... Could anyone review this? (In reply to comment #21) > Created an attachment (id=103222) [details] > Patch
The code changes are all cross platform, removing [Chromium] from title. How is this bug related to bug 41951 and bug 28137? The regression test is a manual one. Can you simulate the click with EventSender?
Comment on attachment 103222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103222&action=review > Source/WebCore/loader/FrameLoader.cpp:198 > + , m_navigationPolicy(NavigationPolicyCurrentTab) Why is this a property of the loader and not of the load itself? > Source/WebCore/loader/FrameLoader.cpp:1173 > + m_navigationPolicy = NavigationPolicyCurrentTab; What about all the other APIs for triggering navigations. As you've written the patch, it seems like they're re-use whatever value m_navigationPolicy happens to be set to.
(In reply to comment #23) > The code changes are all cross platform, removing [Chromium] from title. > > How is this bug related to bug 41951 and bug 28137? I don't think this bug is related to these. This bug is to remove wrong focus() call when a new tab should be opened in background. > > The regression test is a manual one. Can you simulate the click with EventSender? Could you tell me how to check which tab is focused...? This problems includes several tabs, so I don't know how to do that.
> Could you tell me how to check which tab is focused...? The patch is in WebCore, so it should be affecting something other than just browser behavior. WebCore doesn't even know about tabs.
Comment on attachment 103222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103222&action=review > Source/WebCore/loader/FrameLoaderTypes.h:76 > + NavigationPolicyCurrentTab, > + NavigationPolicyNewBackgroundTab, > + NavigationPolicyNewForegroundTab, I'm still not 100% clear on the problem you're trying to solve, but Alexey is correct that the notion of a tab is too specific for WebCore. Generally, the concept of a tab in the browser corresponds to a Page in WebCore. I'm not sure WebCore has any concept of current / background / foreground Pages, however.
Created attachment 105592 [details] Patch
(In reply to comment #26) > > Could you tell me how to check which tab is focused...? > > The patch is in WebCore, so it should be affecting something other than just browser behavior. WebCore doesn't even know about tabs. I've changed the patch so that WebCore does not know about tabs, I believe.
Comment on attachment 105592 [details] Patch Attachment 105592 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9558535 New failing tests: http/tests/security/frameNavigation/xss-DENIED-plugin-navigation.html http/tests/security/frameNavigation/xss-DENIED-targeted-link-navigation.html
Comment on attachment 105592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105592&action=review > Source/WebCore/loader/FrameLoaderClient.h:241 > + virtual bool requiresInitialFocusForTargetFrame(const NavigationAction& action) { return true; } Safari has a user preference whether to activate new tabs and windows. It seems like you may be duplicating existing functionality when adding a new client call.
Comment on attachment 105592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105592&action=review > Source/WebCore/ChangeLog:3 > + Middle clicking a named target wrongly makes a tab foreground. Why is the bug title different from "Middle clicking a named target sets a focus to a previously opened page" ?
Comment on attachment 105592 [details] Patch This patch seems to have numerous unanswered questions, and a break of cr-linux. marking r-. A new patch (and responses on the bug) will be needed to resolve these issues.