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

Add a timezonechange event to Window/WorkerGlobalScope #3047

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

Conversation

littledan
Copy link
Contributor

@littledan littledan commented Sep 15, 2017

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

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

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.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest security/privacy There are security or privacy implications labels Sep 15, 2017
@domenic domenic added the i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. label Sep 15, 2017
@domenic
Copy link
Member

domenic commented Sep 15, 2017

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>
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

@littledan
Copy link
Contributor Author

@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?

@annevk
Copy link
Member

annevk commented Sep 15, 2017

@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 Date works so from that perspective it seems good to at least get aired.

@domenic
Copy link
Member

domenic commented Sep 15, 2017

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.

@rtm
Copy link

rtm commented Sep 15, 2017

Do you mean timezone change, or offset change?

@littledan
Copy link
Contributor Author

@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.

@annevk annevk removed the security/privacy There are security or privacy implications label Sep 15, 2017
@annevk
Copy link
Member

annevk commented Sep 15, 2017

Removing security/privacy label since time zones (and changes to them) are already exposed to script. This doesn't really change anything.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Sep 20, 2017
@litherum
Copy link

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.

@sideshowbarker sideshowbarker added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Oct 18, 2018
@littledan
Copy link
Contributor Author

littledan commented Oct 22, 2018

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.

@aphillips
Copy link
Contributor

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.

@littledan
Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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>
Copy link
Member

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”?

Copy link
Contributor Author

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.

@annevk
Copy link
Member

annevk commented Oct 26, 2018

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.

@littledan
Copy link
Contributor Author

littledan commented Oct 30, 2018

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

  • 😄 for, the timezone change should not take effect until the event is dispatched
  • ❤️ for, the timezone change may take effect earlier
  • 😕 for, I don't care, I just want this feature to happen
  • 👎 for, this feature as a whole seems like overkill

@chrisdavidmills
Copy link

Documentation need recorded at MDN content roadmap — https://trello.com/c/aBs1o6Pg/86-dom-general-enhancements-fx-64

@jungshik
Copy link

https://bugs.chromium.org/p/chromium/issues/detail?id=908550 is a Chromium bug on this feature.

@domenic
Copy link
Member

domenic commented Nov 26, 2018

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.

@littledan
Copy link
Contributor Author

littledan commented Oct 29, 2019

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?

@FrankYFTang
Copy link

I plant to work on it to add it to chrome in 2020Q1 . Looks an important thing for people traveling around.

@littledan
Copy link
Contributor Author

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.

@stephenmcgruer
Copy link
Contributor

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.

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.

@gsnedders
Copy link

As for the WebDriver command, here's an initial draft that adds a new section between “Obsolete features” and “IANA considerations”:

This is massively underspecified. What type is timeZone, even? Is it an integral offset from UTC? Is it a tz database name as a string (e.g. Europe/London)? What is the set of valid values?

@FrankYFTang
Copy link

As for the WebDriver command, here's an initial draft that adds a new section between “Obsolete features” and “IANA considerations”:

This is massively underspecified. What type is timeZone, even? Is it an integral offset from UTC? Is it a tz database name as a string (e.g. Europe/London)? What is the set of valid values?

Sorry, that is my bad in my earlier draft:
How about
"timeZone is a string of the Zone and Link names of the IANA Time Zone Database. All registered Zone and Link names are allowed."
[IANA Time Zone Database] https://www.iana.org/time-zones

@FrankYFTang
Copy link

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

@FrankYFTang
Copy link

@mathiasbynens
I believe the URL need to be "/session/{session id}/time_zone", not "/session/{session id}/timeZone"

see https://w3c.github.io/reporting/#generate-test-report-command
They are using

/session/{session id}/reporting/generate_test_report
not

/session/{session id}/reporting/generateTestReport

@FrankYFTang
Copy link

I suggest we change the webdriver path to
"/session/{session id}/time_zone"
and the name of the parameter to just "zone" to avoid issue of naming convention inconsistency with other specification.

T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Dec 11, 2020
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 <shengfa@google.com>
Reviewed-by: John Chen <johnchen@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836007}
@FrankYFTang
Copy link

ping- could everyone comments on the webDriver extension part of this PR so I can move forward on web-platform-tests/wpt#26555

@domenic
Copy link
Member

domenic commented Dec 16, 2020

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.

@littledan
Copy link
Contributor Author

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.)

@domenic
Copy link
Member

domenic commented Dec 16, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response.
Development

Successfully merging this pull request may close these issues.