Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Don't Center Popups if Size and Width aren't parsed #476

Merged
merged 2 commits into from
Oct 20, 2014
Merged

Conversation

JeffryBooher
Copy link
Contributor

@redmunds redmunds self-assigned this Oct 20, 2014
@redmunds
Copy link
Contributor

@JeffryBooher I'm seeing window correctly open at default size for these cases:

  • window.open("about:blank")
  • window.open("about:blank?height=400")

I was expecting window to open a default size for this case as well, but it still opens minimal window:

  • window.open("about:blank?width=200")

@JeffryBooher
Copy link
Contributor Author

@redmunds what does minimal window mean? I think the OS is positioning the window in all of three cases. What does Windows do if you create a window with a width but height is CW_USEDEFAULT?

@redmunds
Copy link
Contributor

This is what I'm seeing for "default size" (click to see full size):

about-blank1

This is what I'm seeing for "minimal window" case:

about-blank2

@redmunds
Copy link
Contributor

@JeffryBooher

What does Windows do if you create a window with a width but height is CW_USEDEFAULT?

I'm creating window from dev tools console, so CW_USEDEFAULT is not defined and get same result as "minimal window" case.

@JeffryBooher
Copy link
Contributor Author

Yes I see it too.

You're specifying a Width for the Window but not a Height and passing CW_USEDEFAULT to CreateWindow. Since the signature for CreateWindow is: x, y, width, height, in that order, then my guess is when the default impl sees CW_USEDEFAULT for width it uses CW_USEDEFAULT for the height as well but if you don't specify CW_USEDEFAULT for width it expects a valid value for height.

CW_USEDEFAULT is declared as 0x80000000 which is a really small negative number (because the input parameter is signed) which windows would just create a window with the minimum window height in that case.

This seems like an edge case to me -- we could try to clean that up but it seems like this shouldn't be a case that we have to support.

@redmunds
Copy link
Contributor

This is a question for @peterflynn (and maybe @jasonsanjose).

@JeffryBooher
Copy link
Contributor Author

@redmunds no need to wait for @peterflynn -- I've pushed a fix for that case.

@redmunds
Copy link
Contributor

LGTM. Merging.

redmunds added a commit that referenced this pull request Oct 20, 2014
Don't Center Popups if Size and Width aren't parsed
@redmunds redmunds merged commit 0736837 into master Oct 20, 2014
@redmunds redmunds deleted the jeff/fix-9597 branch October 20, 2014 16:09
redmunds added a commit that referenced this pull request Dec 8, 2015
Don't Center Popups if Size and Width aren't parsed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plain about:blank windows are invisible
2 participants