-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add a timezonechange event to Window/WorkerGlobalScope #3047
base: main
Are you sure you want to change the base?
Conversation
Great patch, and I love this feature. I think the real work here to do, as you suggest, is getting implementer feedback on whether they're OK with these semantics. Probably TC39 would be a good place to broach the topic, since even though this is a web platform feature, most of the work in ensuring this consistency would happen in the JS engine, I think. To be clear, the inconsistency you're worried about would only last for a single event loop turn, right? |
source
Outdated
<span>queue a task</span> to <span data-x="concept-event-fire">fire an event</span> named <code | ||
data-x="event-timezonechange">timezonechange</code> at the <code>Window</code> or | ||
<code>WorkerGlobalScope</code> object and wait until that task begins to be executed before | ||
actually returning a new value.</p> |
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.
You want to rephrase this to queue a task that 1. updates the time zone visible to JavaScript (is there some internal slot we could formalize this with?) 2. then dispatches the event.
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.
The wording is cribbed from languagechange. Should I update that as well?
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 would be great. A follow-up issue is fine as well though.
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.
Well, in the end, I'm thinking that we don't actually need to have this timing guarantee.
@domenic Yes, should be just one turn, I think. Not so bad, but I imagine that languagechange's guarantees exist to avoid the same amount of badness. I can talk to implementers and users at TC39 offline, but would this be considered in scope for a main committee presentation? |
@littledan I'd be a little afraid some in TC39 would want to tie this to "Jobs" (which we haven't refactored yet and don't seem the right place), but it does somewhat affect how |
I didn't realize that language change used similar verbiage, hmm. I wonder if it's even true in implementations :). I suppose it would be easier to implement than the corresponding one for times though. I'd go for offline discussions with implementers at TC39, instead of a full presentation to the committee. |
Do you mean timezone change, or offset change? |
@rtm Good question. I mean time zone change. You can read the time zone from Intl, and the text here specifically calls out that case. Even if the offset for 'now' stays the same, offsets for past or future dates may change with a time zone change.The timezone is also represented with various formatting methods. |
Removing security/privacy label since time zones (and changes to them) are already exposed to script. This doesn't really change anything. |
This is a good idea. It doesn't expose any information that isn't already accessible, and would mean that applications wouldn't need to poll. Better for performance, better for battery life. |
I'm not sure if we actually have to delay the timezone update until this callback runs. The delay will cause some additional implementation complexity, and I haven't heard anybody ask for it. I'm thinking to update this patch to remove that requirement. Edit: Updated the patch to remove that aspect of it. |
The W3C I18N WG tasked me with saying that we reviewed this proposal at our TPAC meeting with @littledan and that we support this proposal. |
03f1dfc
to
1fd6eaa
Compare
It's unclear how/whether WPT tests can be written for this feature. I filed web-platform-tests/wpt#13683 to ask for advice. |
source
Outdated
<h5>Time zone changes</h5> | ||
|
||
<p>JavaScript programs can observe the current time zone. <code>Date</code> uses the time zone | ||
through the <code>LocalTZA</code> and <code>DaylightSavingsTA</code> algorithms. |
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.
Abstract operations should be surrounded by <span>
s rather than <code>
s. Ditto below for DefaultTimeZone.
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.
Done
source
Outdated
<span>queue a task</span> to <span data-x="concept-event-fire">fire an event</span> named <code | ||
data-x="event-timezonechange">timezonechange</code> at the <code>Window</code> or | ||
<code>WorkerGlobalScope</code> object. The new time zone must be observable by JavaScript at the | ||
point the task begins, and it may be observable earlier.</p> |
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.
What if the time zone is changed twice, before the task corresponding to the first time zone change runs?
Also, what does it mean for a task to “begin”? I assume it means “beginning running”?
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.
What if the time zone is changed twice, before the task corresponding to the first time zone change runs?
Clarified that the second task is not queued
Also, what does it mean for a task to “begin”? I assume it means “beginning running”?
Right, switched to "begin to run" wording.
I still think it would be better if an agent cached the timezone and we only changed it from a task at the same time the event is dispatched, but I guess I can live with this. I don't think there's precedent for state and the corresponding event to not be synchronized. |
Once we have multi-implementer support, let's poll the implementers to hear their thoughts on these alternatives. FWIW, no one I've talked to about this feature who's not an HTML editor had any strong concerns, use cases or expectations in this area (and many were actually surprised by the idea that the timezone update would be delayed), though I can definitely see the case for this delay. Note that JS implementations which implement tc39/ecma262#778 will have some difficulty using OS APIs for timezone operations, so it's likely to not be as taxing to implement the delay on them as it would be on certain JS implementations which don't follow that patch. It's never too early for an emoji react poll! Vote
|
Documentation need recorded at MDN content roadmap — https://trello.com/c/aBs1o6Pg/86-dom-general-enhancements-fx-64 |
https://bugs.chromium.org/p/chromium/issues/detail?id=908550 is a Chromium bug on this feature. |
Thanks @jungshik. I notice that bug is "untriaged", so I guess it's still not clear whether there is any intent to implement in Chromium. Let us know if that changes, and this feature gets the multi-implementer support it needs. |
Note, there is discussion above about making sure that the new timezone is not reflected in JS until the event is dispatched, which the [now removed] above speculative polyfill (to use the W3C TAG polyfill guide's term) does not do. (Tangent: while this is speculative, I would encourage everyone to not distribute code that squats on part of the event namespace, and instead use a function-based interface.) Do we have implementer support for this feature? |
I plant to work on it to add it to chrome in 2020Q1 . Looks an important thing for people traveling around. |
Fix build + add WebDriver extension command
Thanks for the fixes, @mathiasbynens . I don't understand where it's appropriate to land the WebDriver specification and asked the editors at w3c/webdriver#1559 (comment) , but I don't think this should block the patch from landing; we can refactor it later if needed. The main thing we're missing at this point is the specification for the privacy scheme. |
I am not a WebDriver editor, but can give thoughts from the perspective of someone who has helped a few teams add WebDriver extensions for browser-testing purposes. We have historically encouraged spec authors to add the 'automation/testing support' section to the same specification as the feature that the automation is for. This helps keep the automation APIs 'in sync' with the feature APIs, and also keeps the main WebDriver spec leaner and clearer (rather than be a laundry list of extensions). There is significant prior art here; permissions, reporting, sensors, webauthn, and background fetch all have webdriver extensions in their spec. |
This is massively underspecified. What type is |
Sorry, that is my bad in my earlier draft: |
Anyone object to the web driver part of the PR? Any one support the web driver part of the PR? I need some signal to proceed the change the WPT test |
@mathiasbynens see https://w3c.github.io/reporting/#generate-test-report-command /session/{session id}/reporting/generate_test_report /session/{session id}/reporting/generateTestReport |
I suggest we change the webdriver path to |
Change timeZone to time_zone
add description about time_zone
Sync w/ whatwg/html#3047 Bug: 1144403 Change-Id: I67694714ee5799e2e731b4d0534a7c182871fc23 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2574533 Reviewed-by: Shengfa Lin <[email protected]> Reviewed-by: John Chen <[email protected]> Commit-Queue: Frank Tang <[email protected]> Cr-Commit-Position: refs/heads/master@{#836007}
ping- could everyone comments on the webDriver extension part of this PR so I can move forward on web-platform-tests/wpt#26555 |
It's not clear who you're looking for review on this from. From an editors point of view, this isn't really reviewable because it has conflicts with master and build errors. We'd also like to get some agreement (i.e. self-review) from the various contributors, which at this point look to be @littledan, @mathiasbynens, and @FrankYFTang, before doing editor review. |
I don't feel qualified to review the WebDriver parts of this patch, but I believe @mathiasbynens and @FrankYFTang collaborated on the WebDriver changes here. @domenic Can you say more about what kind of self-review you're looking for? (As I recently noted, this patch is not ready to land until it adds these privacy fixes. I'd welcome contributions here; I don't have time to do this work right now.) |
Thanks Dan, your "As I recently noted, this patch is not ready to land until it adds these privacy fixes." is the kind of self-review I was hoping for: i.e., figuring out whether all the folks involved thought this patch was ready for editor review or not. Since it sounds like there's significant work outstanding that may change the spec text in a variety of ways, we'll hold off on reviewing until that is ready. |
The current timezone is visible to JavaScript and changes over time.
Changes may be useful to note, e.g., in a long-running calendar or mail
application that may want to update dates and times without refreshing
when the user resumes using the application after travel.
Currently, to accomplish that, a webapp would have to poll, e.g.,
by repeatedly calling
Intl.DateTimeFormat().resolvedOptions().timeZone
.With this patch, an event would instead notify the application.
In this patch, the time zone may change before the task is run, but
must be changed at the point the task is run. A previous version included
additional guarantees, that the time zone would not be changed until the
start of the task, but these semantics add implementation complexity and
no concrete use case was found for the greater guarantees.
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.