Bug 61167 – Middle clicking a named target sets a focus to a previously opened page

• Component: WebKit Misc.
• Status: UNCONFIRMED

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

Created attachment 94168 [details]
HTML file containing named target.

[Alexey Proskuryakov]

I cannot reproduce this in Safari 5.0.5 on Mac OS X . What browser have you been testing with?

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 94182 [details]
Another file.

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

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.

[Dominic Cooney]

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?

[Shinya Kawanaka]

(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.

[Alexey Proskuryakov]

> 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.

[Shinya Kawanaka]

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...

[Matt Hickford]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

I made this issue chromium-specific. 

The Safari's problem is moved to
https://bugs.webkit.org/show_bug.cgi?id=65454

[Shinya Kawanaka]

Created attachment 102492 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 103016 [details]
Patch

[Hajime Morrita]

Comment on attachment 103016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103016&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Shinya Kawanaka  <[e-mail]>

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?

[Shinya Kawanaka]

Created attachment 103215 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103215 [details]
Patch

Attachment 103215 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9215177

[Shinya Kawanaka]

Created attachment 103222 [details]
Patch

[Shinya Kawanaka]

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

[Alexey Proskuryakov]

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?

[Adam Barth]

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.

[Shinya Kawanaka]

(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.

[Alexey Proskuryakov]

> 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.

[Adam Barth]

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.

[Shinya Kawanaka]

Created attachment 105592 [details]
Patch

[Shinya Kawanaka]

(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.

[WebKit Review Bot]

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

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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" ?

[Eric Seidel (no email)]

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.