Skip to content

port pattern canonicalization seems wrong after all #260

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

Closed
annevk opened this issue Mar 7, 2025 · 3 comments
Closed

port pattern canonicalization seems wrong after all #260

annevk opened this issue Mar 7, 2025 · 3 comments

Comments

@annevk
Copy link
Member

annevk commented Mar 7, 2025

What is the issue with the URL Pattern Standard?

I'd like to revisit #239. cc @anonrig

new URLPattern({ "protocol": "http", "port": "80 " })

The expectation of this test is that the port gets elided because 80 is the default port for "http".

This is the set of steps:

  1. https://urlpattern.spec.whatwg.org/#dom-urlpattern-urlpattern-input-options The constructor invokes initialize.
  2. https://urlpattern.spec.whatwg.org/#urlpattern-initialize Initialize This invokes create.
  3. Create invokes https://urlpattern.spec.whatwg.org/#process-a-urlpatterninit (notably with "pattern" as second argument)
  4. In step 16 this invokes https://urlpattern.spec.whatwg.org/#process-port-for-init but note that it returns the value immediately due to type being "pattern". So result["port"] is "80 " including trailing space.
  5. Then we go back up and hit step 6 of https://urlpattern.spec.whatwg.org/#url-pattern-create but this won't elide the port either as it has trailing whitespace. (A naïve implementation might strip the whitespace here, which is maybe what happened when the test got written?)
  6. Then we hit step 13 but at this point we no longer have protocol as context so port elision can no longer occur.

cc @jeremyroman cc @sisidovski

@anonrig
Copy link
Contributor

anonrig commented Mar 11, 2025

You're right. The spec should mention trim c0 whitespace before checking if protocol's port is equal to port. Here's the Ada implementation that follows the test but fails to follow the specification:

  // If processedInit["protocol"] is a special scheme and processedInit["port"]
  // is a string which represents its corresponding default port in radix-10
  // using ASCII digits then set processedInit["port"] to the empty string.
  // TODO: Optimization opportunity.
  if (scheme::is_special(*processed_init->protocol)) {
    std::string_view port = processed_init->port.value();
    helpers::trim_c0_whitespace(port);
    if (std::to_string(scheme::get_special_port(*processed_init->protocol)) ==
        port) {
      processed_init->port->clear();
    }
  }

I recommend adding trim whitespace behavior to the spec itself.

@sisidovski
Copy link
Collaborator

Yeah I believe the current wpt is the intuitive behavior for developers.

Maybe we can add the step removing c0 whitespace from the input in https://urlpattern.spec.whatwg.org/#process-port-for-init.

@annevk
Copy link
Member Author

annevk commented Mar 19, 2025

This has been resolved in web-platform-tests/wpt#51316 based on a discussion in #261.

@annevk annevk closed this as completed Mar 19, 2025
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 23, 2025
…lated test, a=testonly

Automatic update from web-platform-tests
URLPattern: correct port and hostname-related test

For whatwg/urlpattern#260 (port) and whatwg/urlpattern#252 (hostname).
--

wpt-commits: a8c62524f649ca5fa69e0f7411f16955edadb851
wpt-pr: 51316
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Mar 26, 2025
…lated test, a=testonly

Automatic update from web-platform-tests
URLPattern: correct port and hostname-related test

For whatwg/urlpattern#260 (port) and whatwg/urlpattern#252 (hostname).
--

wpt-commits: a8c62524f649ca5fa69e0f7411f16955edadb851
wpt-pr: 51316
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 28, 2025
…lated test, a=testonly

Automatic update from web-platform-tests
URLPattern: correct port and hostname-related test

For whatwg/urlpattern#260 (port) and whatwg/urlpattern#252 (hostname).
--

wpt-commits: a8c62524f649ca5fa69e0f7411f16955edadb851
wpt-pr: 51316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants