Bug 56682 – Opening link with unspecific hash in a new tab (except context menu) twice in a row results in hash for current window changing

• Component: New Bugs
• Status: UNCONFIRMED

[Harald Glatt]

STR:
1. Place a link in a html document looking like this: <a href="#hash">some unspecific hash</a>
2. Open the html document in Safari or Chrome (Windows and Mac OS confirmed)
3. Command + Click the link twice in a row (Control + Click on Windows), doesn't have to be quick
4. Observe first click correctly opens a new tab with the URL having the hash attached.
5. Observe second click changing the hash in the url of the current window.
6. Observe the second and each consecutive click failing to open a new tab.

Additional Info:
- Happens on all platforms (Windows and Mac OS confirmed) on both Safari and Chrome.
- Happens with Safari 5.1 using WebKit2 on Mac OS 10.7.
- Happens on Windows when using the middle mouse button to open a link in a new tab instead of the keyboard
- Doesn't happen using the context menu for opening in a new tab. (Both Chrome and Safari, all platforms)
- Links with a href in the following style are not affected: <a href="index.html#hash">some precise hash</a>

Without knowledge of the WebKit code I am unable to give a component that might be causing this bug, I'm sorry about that.

[ChangSeok Oh]

I could represent this issue on Mac and GTK prot.(They're just available systems to me :p)
By the way, the resean is a little different between Mac and GTK port.

First, Mac port.
If an anchor has a hash value for 'href' attribute, new request is swallowed regardless of ctrl & shift button. Because previous request & new request are identical. 
Actually WebCore::PolicyChecker::checkNavigationPolicy has checked it.

The other case, GTK port.
GTK port also shares Mac port reason.
In addition, application layer impelementation is more needed to open link in new window.
I'm not sure, but it looks like that there is no signal handler for "navigation-policy-decision-requested" and "navigation-requested".
I guess it is reminded for browser application developer to implement.
When I tested using by just GtkLauncher, I was not able to open link in new window with ctrl, shift key though anchor had full http address.

I think that we need to take care of first reason.
Blocking duplicated request is reasonable in same window, but it is not for different window.

Any idea?

[ChangSeok Oh]

Created attachment 87154 [details]
Test case

[ChangSeok Oh]

Created attachment 87157 [details]
Proposed patch

[Harald Glatt]

Looking over your patch I noticed that you are directly handling the ctrl or meta button state. This means it would not be fixed for using the middle mousebutton click on windows, right? This should be included...

[Adam Barth]

Comment on attachment 87157 [details]
Proposed patch

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

This patch lacks a test.  Generally speaking, every patch requires a test.

> Source/WebCore/loader/PolicyChecker.cpp:69
> +        const UIEventWithKeyState* keyStateEvent = findEventWithKeyState(const_cast<Event*>(action.event()));

const_cast ?  That doesn't seem right.

> Source/WebCore/loader/PolicyChecker.cpp:74
> +        if (!(keyStateEvent && (keyStateEvent->ctrlKey() || keyStateEvent->metaKey()))) {
> +            function(argument, request, 0, true);
> +            loader->setLastCheckedRequest(request);
> +            return;
> +        }

PolicyChecker shouldn't know anything about ctrlKey or metaKey.  I think this patch is improperly factored.

[ChangSeok Oh]

Comment on attachment 87157 [details]
Proposed patch

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

Thanks for review! :)
I've prepared a test case for this.

>> Source/WebCore/loader/PolicyChecker.cpp:69
>> +        const UIEventWithKeyState* keyStateEvent = findEventWithKeyState(const_cast<Event*>(action.event()));
> 
> const_cast ?  That doesn't seem right.

Done.

>> Source/WebCore/loader/PolicyChecker.cpp:74
>> +        }
> 
> PolicyChecker shouldn't know anything about ctrlKey or metaKey.  I think this patch is improperly factored.

Well, if PolicyChecker shouldn't, I think FrameLoader can check this.
Anyway, somewhere should check this.

[ChangSeok Oh]

Created attachment 90367 [details]
Proposed patch

[Adam Barth]

Comment on attachment 90367 [details]
Proposed patch

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

> Source/WebCore/loader/FrameLoader.cpp:1381
> +    if (event && event->isMouseEvent()) {
> +        RefPtr<MouseEvent> mouseEvent = static_pointer_cast<MouseEvent>(event);
> +#if PLATFORM(MAC)
> +        if (mouseEvent->metaKey()) {
> +#else
> +        if (mouseEvent->ctrlKey() || mouseEvent->button() == MiddleButton) {
> +#endif
> +            policyChecker()->checkNewWindowPolicy(action, FrameLoader::callContinueLoadAfterNewWindowPolicy,
> +                request, formState.release(), frameName, this);
> +            return;
> +        }

This is the wrong place to do this work.  The FrameLoader shouldn't understand anything about the meta or ctrl keys.  Even worse, it shouldn't be encoding platform-specific behavior!

[ChangSeok Oh]

(In reply to comment #8)
> (From update of attachment 90367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90367&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1381
> > +    if (event && event->isMouseEvent()) {
> > +        RefPtr<MouseEvent> mouseEvent = static_pointer_cast<MouseEvent>(event);
> > +#if PLATFORM(MAC)
> > +        if (mouseEvent->metaKey()) {
> > +#else
> > +        if (mouseEvent->ctrlKey() || mouseEvent->button() == MiddleButton) {
> > +#endif
> > +            policyChecker()->checkNewWindowPolicy(action, FrameLoader::callContinueLoadAfterNewWindowPolicy,
> > +                request, formState.release(), frameName, this);
> > +            return;
> > +        }
> 
> This is the wrong place to do this work.  The FrameLoader shouldn't understand anything about the meta or ctrl keys.  Even worse, it shouldn't be encoding platform-specific behavior!

Then, what do you think about this way like following...??
First, we add new API (like .. needToOpenNewWindow?) in PlatformMouseEvent & MouseEvent. This API is just getter function of boolean value m_needToOpenNewWindow which is decided when PlatformMouseEvent is initialized.
Second, we check whether new window should be opened or not with this common API in FrameLoader::loadURL.

I think we can avoid decoding platform specific behavior with this way in FrameLoader.

[Adam Barth]

I apologize that I do not have time to help you with your patch.  Hopefully you'll be able to find someone else who will be able to answer your questions.