Bug 119633 – REGRESSION (r142755): window.open creates an invisible window when width and height are 0

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Alexey Proskuryakov]

javascript:window.open("http://www.apple.com", "", "height=0,width=0") creates an invisible window in Safari now.

<rdar://problem/14693930>

[Alexey Proskuryakov]

Treating (0, 0) as default size falls apart because windowRect height ends up being 24 here:

    if (features.heightSet)
        windowRect.setHeight(features.height + (windowRect.height() - viewportSize.height()));

[Alexey Proskuryakov]

window.resizeTo is broken too.

[Alexey Proskuryakov]

I'm thinking that r142755 should be rolled out entirely, and replaced with a fix in createWindow() function in FrameLoader.cpp:

-    if (features.widthSet)
+    if (features.widthSet && features.width)
        windowRect.setWidth(features.width + (windowRect.width() - viewportSize.width()));
-    if (features.heightSet)
+    if (features.heightSet && features.height)
        windowRect.setHeight(features.height + (windowRect.height() - viewportSize.height()));

Allan, does this make sense?

[Alexey Proskuryakov]

Created attachment 208451 [details]
proposed fix

[Darin Adler]

Comment on attachment 208451 [details]
proposed fix

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

> Source/WebCore/ChangeLog:3
> +        REGRESSION (r142755): window.open creates an invisible window when width and height are 0

Seems like the bug is when width *or* height is zero. What about small widths and heights that are not zero?

[kov's GTK+ EWS bot]

Comment on attachment 208451 [details]
proposed fix

Attachment 208451 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1410299

[Allan Sandfeld Jensen]

I don't understand how the browser is supposed to be able to guess if default size was requested when the get an openWindow call.

The issue is this:
In many browsers windows that request a specific size are opened in a separate window, but windows that request default size are opened in a tab. This is decided outside of WebKit, and thus WebKit needs to tell the browser that default was requested. If 0x0 is not used, what is?

[Alexey Proskuryakov]

Looks like Gtk bot is broken now.

> Seems like the bug is when width *or* height is zero. What about small widths and heights that are not zero?

These are handled in DOMWindow::adjustWindowRect, which enforces a minimum size. Width and height are adjusted to be at least 100.

> In many browsers windows that request a specific size are opened in a separate window, but windows that request default size are opened in a tab.

When WebKit creates a window, it's before any of this code is run (oldPage->chrome().createWindow() in FrameLoader.cpp). The window is always created with a default size, by definition of default size, and is then resized by the code this patch touches. Afterwards, it's displayed with page->chrome().show().

This has to work in all WebKit based browsers as long as window.open(url, "", "") works with no explicit size. We just read the current size from client using page->chrome().windowRect() and page->chrome().pageRect(), adjust it to be on screen and to not be too small, and feed back to the client. I find it a bit strange how this logic is split between WebKit and client, but it works.

Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then.

[Alexey Proskuryakov]

Comment on attachment 208451 [details]
proposed fix

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131
> -    if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty())
> +    if (!ewk_view_setting_enable_auto_resize_window_get(m_view))

Actually, I think that this code used to break moving the window to screen if it had default size, but an offscreen position. But perhaps that can never happen in practice.

[Alexey Proskuryakov]

> I don't understand how the browser is supposed to be able to guess if default size was requested when the get an openWindow call.

To answer your question more directly, WebCore does pass all the requested features to the client, so it can do anything even upfront in createWindow(). It is kind of strange that WebCore enforces additional constraints after the fact, I don't know why we do this.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then.

Yes all of them except Safari apparently.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #10)
> > Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then.
> 
> Yes all of them except Safari apparently.

Well, specifically the classic desktop browsers implementing tabs. Default size windows opening in tabs is expected behavior on a lot of sites that can control whether a new window should popup or not that way. WebKit for years requsted a window of size (0,0) if no size was set, so that became how the browsers implemented the standard behavior. The minimum size for unset window size was only enforced for a few months before I modified it to let 0 through again. Did something change in Safari to rely on the minimum in the timespan it was enforced?

[Alexey Proskuryakov]

> WebKit for years requsted a window of size (0,0) if no size was set

The windows is created by this method:

Page* Chrome::createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const NavigationAction& action) const
{
    Page* newPage = m_client->createWindow(frame, request, features, action);
...
}

How was the (0, 0) size passed? The size is in WindowFeatures, but neither your not my patch change anything about that structure content. We only change later calls that resize the already created window.

> The minimum size for unset window size was only enforced for a few months before I modified it to let 0 through again.

Do you know when this was added? As mentioned before, I'm surprised that any of this logic is in WebCore.

[WebKit Commit Bot]

Comment on attachment 208451 [details]
proposed fix

Clearing flags on attachment: 208451

Committed r153913: <http://trac.webkit.org/changeset/153913>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

(In reply to comment #13)
> Do you know when this was added? As mentioned before, I'm surprised that any of this logic is in WebCore.

The adjustment was there before but the part that overwrote default 0,0 with minimum came from http://trac.webkit.org/changeset/134586

[Alexey Proskuryakov]

Thank you for the pointer. Based on what I learned looking at this code today, it's probably wrong to call adjustWindowRect when creating a new window, and in general, it seems that logic added in r134586 should be client's responsibility.