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

Wrong Web IDL type for URLs in HTML Standard #10895

Open
1 task
dontcallmedom-bot opened this issue Jan 6, 2025 · 4 comments · May be fixed by #10916
Open
1 task

Wrong Web IDL type for URLs in HTML Standard #10895

dontcallmedom-bot opened this issue Jan 6, 2025 · 4 comments · May be fixed by #10916

Comments

@dontcallmedom-bot
Copy link

While crawling HTML Standard, wrong Web IDL type for URLs:

  • attributes url and src in interface NotRestoredReasons uses DOMString instead of recommended USVString for URLs

Cc @dontcallmedom @tidoust

This issue was detected and reported semi-automatically by Strudy based on data collected in webref.

@annevk
Copy link
Member

annevk commented Jan 10, 2025

@rubberyuzu could you take a look at this?

@domenic
Copy link
Member

domenic commented Jan 10, 2025

Since these are getters, and are only ever populated by serializing URLs, it should be a non-observable change to just update the spec to align with best practices.

@domenic
Copy link
Member

domenic commented Jan 15, 2025

Actually, this is observable for src per the current spec, considering a case like http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=13395 where the actual src="" attribute has been made to contain an unpaired surrogate.

However, this made me wonder if we wrote the current spec correctly. It currently says

Set notRestoredReasonsForDocument's src to the value of document's node navigable's container's src attribute.

Which means it's getting the literal value of the src content attribute.

But I suspect it would be more useful to get the value reflected by the src IDL attribute, which contains the result of parsing-then-serializing the URL. I also suspect that might be how it's implemented in Chromium, since it's pretty easy to just do iframe_element->src().

@rubberyuzu is out on leave for some time, but perhaps @fergald can take a look at this? The action is:

  • Clarify what the intended behavior and/or Chromium implementation is, in terms of "literal src="" content attribute value" vs. "parsed-then-serialized src IDL attribute value".
  • Write a test for the above, if there isn't one already.
  • Depending on the intended behavior:
    • If it's content attribute value: probably no action, and this is a false positive from the bot?
    • If it's IDL attribute value: update the spec to say that.

@domenic
Copy link
Member

domenic commented Jan 15, 2025

Actually, I realized I can check the tests myself. They reveal the Chromium implementation does "parsed-then-serialized src IDL attribute value". Since this makes more sense to me anyway, I'll just send a quick PR to reflect that in the spec.

No action needed from @fergald, sorry for the ping.

domenic added a commit that referenced this issue Jan 15, 2025
url was previously listed as a DOMString, but it should be a USVString. This change is not observable since it always contained a serialized URL anyway.

src was previously listed as containing the literal value of the src="" content attribute. However, the intended behavior was to contain the parsed-then-serialized value of the src IDL attribute, i.e., the result after content/IDL attribute reflection. Fix the spec to reflect that, which allows also changing it to USVString.

Finally, also account for the fact that sometimes content attributes can be missing, in which case these properties need to return the empty string. (Not null, which is reserved for the top level or non-iframe child cases.)

Closes #10895.
@domenic domenic linked a pull request Jan 15, 2025 that will close this issue
6 tasks
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