Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change URL parsing sequence for window open steps #10683

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 7, 2024

Previously, we would only check at the last minute. This would be buggy in the case where we create a new navigable, and then throw an exception. Instead we should be sure to throw before creating a new navigable.

In addition to fixing this somewhat-obviously-wrong bug, this also causes URL parsing to happen even in cases where a named window is targeted, but no window with that name exists. Previously, the window open steps would do nothing in such cases; now, they throw an exception when given an unparseable URL.

Closes #10681.

@shannonbooth, your review would be appreciated. Also, are you interested in helping to write web platform tests for the normative change described above, about adding validation? I checked by source inspection that it matches how Chromium behaves, but depending on the results we might need to do more work to match the browser majority here.

  • At least two implementers are interested (and none opposed):
    • This matches Chromium
    • TBD via web platform tests if it matches the others
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: N/A
    • Gecko: TBD
    • WebKit: TBD
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): N/A
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): N/A
  • MDN issue is filed: N/A, too minor
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)


/nav-history-apis.html ( diff )

Previously, we would only check at the last minute. This would be buggy in the case where we create a new navigable, and then throw an exception. Instead we should be sure to throw before creating a new navigable.

In addition to fixing this somewhat-obviously-wrong bug, this also causes validation to happen in cases where a named window is targeted, but no window with that name exists. Previously, the window open steps would do nothing in such cases; now, they throw an exception when given an invalid URL.

Closes #10681.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this works out testing-wise as this seems like a very good change.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@shannonbooth
Copy link

The changes look good to me, and I also hope that testing on other implementations works out as the normative change is intuitive to me.

Also, are you interested in helping to write web platform tests for the normative change described above, about adding validation? I checked by source inspection that it matches how Chromium behaves, but depending on the results we might need to do more work to match the browser majority here.

I will give it a shot when I get some time! Not fully familiar with the process, but will figure it out :^)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

window.open() checks for invalid URL after creating a new traversable
3 participants