feat: enable innerWidth and innerHeight for window open by mlaurencin · Pull Request #46749 · electron/electron

mlaurencin · 2025-04-23T21:11:11Z

Description of Change

Previously, our implementation of window.open did not allow for innerWidth and innerHeight as available options for the window.open features string. As per the spec, these options should be available and should also be equivalent to setting the width and height options: https://developer.mozilla.org/en-US/docs/Web/API/Window/open#width

This PR specifically adds in the innerWidth and innerHeight options to set the innerWidth and innerHeight of the window respectively. A follow-up PR will be required to make the width and height options match this functionality, as this will be a breaking change (they currently equate to setting outerWidth and outerHeight respectively).

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant API documentation, tutorials, and examples are updated and follow the documentation style guide
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: added innerWidth and innerHeight options for window.open

shell/browser/native_window_mac.mm

ckerr · 2025-04-24T19:46:23Z

shell/browser/native_window_mac.mm

+    if (inner_width)
+      width = (inner_width < 100) ? 100 : inner_width;
+    if (inner_height)
+      height = (inner_height < 100) ? 100 : inner_height;


Better to use foo = std::max(100, inner_foo) here

We're still allowing a value of <100 to be inherited from either width or height via options::kWidth. Is that OK?

Maybe we should update the docs to point out that a minimum of 100 is required?

It's also not clear to me why 100 is being used here?

@codebytere it's in the spec:

https://developer.mozilla.org/en-US/docs/Web/API/Window/open#width

Specifies the width of the content area, including scrollbars. The minimum required value is 100.

But you're right, that's not clear unless one reads the spec or this thread. Maybe a symbolic constant name could be used to make it clearer, e.g.

constexpr int kMinSizeReqdBySpec = 100;

ah cool - ty! perhaps a comment with spec link?

I think since this is supposed to be inline with the window.open spec, our docs do not need to also state that it adheres to this requirement specifically. Although, I think linking to the spec in the code is helpful to understand what might seem like an arbitrary value at first glance.

lib/browser/parse-features-string.ts

spec/chromium-spec.ts

requested changes have been made

* feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>

trop · 2025-05-09T16:04:20Z

I have automatically backported this PR to "36-x-y", please check out #47038

trop · 2025-05-09T16:04:25Z

I have automatically backported this PR to "37-x-y", please check out #47039

* feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews

trop · 2025-05-09T18:31:46Z

@mlaurencin has manually backported this PR to "35-x-y", please check out #47045

feat: enable innerWidth and innerHeight for window open (#46749) * feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>

) * feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews

feat: enable innerWidth and innerHeight for window open (#46749) * feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>

* feat: enable innerWidth and innerHeight for window open * update comment for added special innerWidth and innerHeight * update 100 min spec requirement handling * update testing to include getContentSize * update macOS min requirement handling * adjust refactored consts * update const values from nativewindowviews

mlaurencin added semver/patch backwards-compatible bug fixes target/34-x-y PR should also be added to the "34-x-y" branch. target/35-x-y PR should also be added to the "35-x-y" branch. target/36-x-y PR should also be added to the "36-x-y" branch. labels Apr 23, 2025

mlaurencin requested review from codebytere and jkleinsc April 23, 2025 21:11

electron-cation bot added the new-pr 🌱 PR opened recently label Apr 23, 2025

mlaurencin changed the title ~~enable innerWidth and innerHeight for window open~~ feat: enable innerWidth and innerHeight for window open Apr 23, 2025

MarshallOfSound previously requested changes Apr 23, 2025

View reviewed changes

shell/browser/native_window_mac.mm Show resolved Hide resolved

feat: enable innerWidth and innerHeight for window open

6b01c06

mlaurencin force-pushed the enable-window-open-inner-separate branch from 612bdf6 to 6b01c06 Compare April 24, 2025 15:01

ckerr reviewed Apr 24, 2025

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened recently label Apr 24, 2025

codebytere reviewed Apr 25, 2025

View reviewed changes

lib/browser/parse-features-string.ts Outdated Show resolved Hide resolved

spec/chromium-spec.ts Show resolved Hide resolved

update comment for added special innerWidth and innerHeight

1d809ff

github-actions bot added the target/37-x-y PR should also be added to the "37-x-y" branch. label Apr 29, 2025

mlaurencin added 3 commits May 1, 2025 20:29

update 100 min spec requirement handling

cb93d40

update testing to include getContentSize

ae671c5

update macOS min requirement handling

c4f8cb1

mlaurencin requested review from MarshallOfSound, ckerr and codebytere May 5, 2025 14:12

mlaurencin and others added 3 commits May 7, 2025 11:28

Merge branch 'main' into enable-window-open-inner-separate

836b07f

adjust refactored consts

23888ab

update const values from nativewindowviews

0fd0687

ckerr approved these changes May 7, 2025

View reviewed changes

jkleinsc approved these changes May 9, 2025

View reviewed changes

jkleinsc merged commit b9f0aeb into main May 9, 2025
58 checks passed

jkleinsc deleted the enable-window-open-inner-separate branch May 9, 2025 16:03

trop bot removed the target/34-x-y PR should also be added to the "34-x-y" branch. label May 9, 2025

trop bot added needs-manual-bp/34-x-y and removed target/35-x-y PR should also be added to the "35-x-y" branch. labels May 9, 2025

This was referenced May 9, 2025

feat: enable innerWidth and innerHeight for window open #47038

Merged

feat: enable innerWidth and innerHeight for window open #47039

Merged

trop bot added the needs-manual-bp/35-x-y label May 9, 2025

trop bot added in-flight/36-x-y in-flight/37-x-y and removed target/36-x-y PR should also be added to the "36-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. labels May 9, 2025

mlaurencin removed the needs-manual-bp/34-x-y label May 9, 2025

mlaurencin mentioned this pull request May 9, 2025

feat: enable innerWidth and innerHeight for window open (#46749) #47045

Merged

trop bot added in-flight/35-x-y and removed needs-manual-bp/35-x-y labels May 9, 2025

trop bot added merged/37-x-y PR was merged to the "37-x-y" branch. and removed in-flight/37-x-y labels May 14, 2025

trop bot added merged/35-x-y PR was merged to the "35-x-y" branch. and removed in-flight/35-x-y labels May 15, 2025

trop bot added merged/36-x-y PR was merged to the "36-x-y" branch. and removed in-flight/36-x-y labels May 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable innerWidth and innerHeight for window open#46749

feat: enable innerWidth and innerHeight for window open#46749
jkleinsc merged 8 commits intomainfrom
enable-window-open-inner-separate

mlaurencin commented Apr 23, 2025 •

edited by ckerr

Loading

Uh oh!

Uh oh!

ckerr Apr 24, 2025 •

edited

Loading

Uh oh!

codebytere Apr 25, 2025

Uh oh!

ckerr Apr 25, 2025

Uh oh!

codebytere Apr 25, 2025 •

edited

Loading

Uh oh!

mlaurencin Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trop bot commented May 9, 2025

Uh oh!

trop bot commented May 9, 2025

Uh oh!

trop bot commented May 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

mlaurencin commented Apr 23, 2025 • edited by ckerr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

Uh oh!

ckerr Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codebytere Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

ckerr Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

codebytere Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaurencin Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trop bot commented May 9, 2025

Uh oh!

trop bot commented May 9, 2025

Uh oh!

trop bot commented May 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mlaurencin commented Apr 23, 2025 •

edited by ckerr

Loading

ckerr Apr 24, 2025 •

edited

Loading

codebytere Apr 25, 2025 •

edited

Loading