-
Notifications
You must be signed in to change notification settings - Fork 173
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
Update <a> element monkey patch with content attributes #140
Conversation
Includes the list of content attributes to add, and specifies that they must be reflected from the idl attributes
@jyasskin could you take a look? |
@@ -49,29 +49,45 @@ conversion domain. | |||
|
|||
# HTML monkeypatches # {#html-monkeypatches} | |||
|
|||
Add the following content attributes to the <{a}> element: | |||
Add the following <a spec=html>content attributes</a> to the <{a}> element: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should file a bug against HTML to check if there's a better way to do this than referring to a bunch of non-exported dfn
s. E.g. HTML could export these definitions.
I believe Anne also likes us to file an HTML issue saying that this monkeypatch exists, like whatwg/fetch#784.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general monkeypatch specs need a lot of similar references to non-exported definitions; that's probably the best way.
index.bs
Outdated
[CEReactions] attribute DOMString attributiondestination; | ||
[CEReactions] attribute DOMString attributionsourceeventid; | ||
[CEReactions] attribute DOMString attributionreportto; | ||
[CEReactions] attribute DOMString attributionexpiry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glancing over the HTML spec, it does look like numeric content attributes get reflected into numeric IDL types, so you should probably either make this an unsigned long
or add a definition for unsigned long long
reflection into HTML. 32-bit numbers of milliseconds give us 49 days of expiration, which cuts it a little close to the current maximum expiration times, so I lean toward sending a change to HTML.
@domenic, before I make @johnivdel do extra work, does this match your sense of the right thing to do?
Assuming this is the right thing to do, I would commit this as an unsigned long long
before you get the change into HTML, even though that's formally broken, because it's unlikely to cause implementation divergence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had changed this to a DOMString to align with attributionsourceeventid
, which also represents a 64 bit unsigned integer.
Using unsigned long long runs into Number.MAX_SAFE_INTEGER
as noted on #123, and so we decided to accept the value as a string for simplicity. Although perhaps I am misunderstanding the relationship between JavaScript & WebIDL here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I tend to agree that normally things reflect as numbers, but if there is a legitimate case where the number would be outside the max-safe-integer range, then either strings or bigints would be correct. Bigints would be unprecedented but perhaps better.
On the other hand, maybe it's OK for the number to stay inside the 2^53 range, in which case creating unsigned long long reflection would make sense. Hmm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
253 milliseconds is 285,427 years, so it's probably fine for an expiration time. It's not as good for an event ID, but those are IDs rather than things the Javascript might do math on, which is a plausible reason to keep them different.
This could be a good argument for giving the reflection machinery a knob similar to "limited to only non-negative numbers" to control whether the reflection throws if it's given a non-safe number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree w/ @jyasskin that 2^53 should be fine.
In this case, both an unsigned long long
and a long long
"limited to only non-negative integers" would support the same range of values given the 2^53 limit. Is using the unsigned type still preferable in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we have "long
limited to only non-negative integers", so I don't really have a preference between you using long long
limited… or unsigned long long
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As to @jyasskin's question, I suspect that most HTML interfaces were developed before Web IDL really existed, and so a lot of things used long
semantics in the implementation, with custom checks.
I think defining "long long
limited only to non-negative numbers" would probably be best, both for consistency and for web developers. In particular, if we had unsigned long long
reflection, then
el.attributionExpiry = -1;
would behave the same as el.attributionExpiry = 2**64 - 1
. Whereas if you did "long long
limited only to non-negative numbers", it would throw an "IndexSizeError
" DOMException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I've updated the IDL to long long.
What's the best way to move forward on spec'ing the long long reflection? Given that it would be unused does it make more sense to add it to this spec? Or just file a PR against HTML that would be necessary if these monkeypatches were resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO adding it to this spec, with a note that you intend to upstream it to HTML along with the rest of the monkeypatches, makes sense.
Co-authored-by: Jeffrey Yasskin <jyasskin@chromium.org>
index.bs
Outdated
[CEReactions, Reflect] attribute DOMString attributionsourceeventid; | ||
[CEReactions, Reflect] attribute DOMString attributionreportto; | ||
[CEReactions, Reflect] attribute unsigned long long attributionexpiry; | ||
[CEReactions] attribute DOMString attributiondestination; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDL attributes use camel case, so e.g. attributionDestination
, attributionSourceEventId
, attributionReportTo
, attributionExpiry
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks! For John, the rules are in https://w3ctag.github.io/design-principles/#casing-rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Includes the list of content attributes to add, and specifies that they must be reflected from the idl attributes as discussed on #133
Preview | Diff