Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Input fields don't emit (and thus round-trip) offset value when binding against DateTimeOffset fields #6648

Closed
DamianEdwards opened this issue Aug 12, 2017 · 30 comments

Comments

@DamianEdwards
Copy link
Member

Was working on the ASP.NET Core App-building Workshop and discovered an interesting issue when model binding to properties of type DateTimeOffset, especially when the application is running on a machine with a different system locale (and thus default time zone) than the DateTimeOffset values being edited.

Consider a Tag Helper use like <input asp-for="StartTime" />, where StartTime is defined as public DateTimeOffset StartTime { get; set; } on the model. The rendered HTML for the input field will not include the offset value: <input class="form-control" type="datetime-local" id="StartTime" name="StartTime" value="2017-08-14T09:00:00.000" /> . When this is posted back, the model bound value will now have an offset of the machine's system locale, even if the field wasn't changed, resulting in a different value.

We need to consider what the correct behavior of the helpers should be here, and how it interacts with the HTML5 datetime-local field type. Or perhaps we need specific guidance (and maybe helpers/binders) for handling DateTimeOffset values.

@rynowak @Eilon

@Eilon Eilon added this to the 2.1.0 milestone Aug 14, 2017
@Eilon
Copy link
Member

Eilon commented Aug 14, 2017

@dougbu can you take a look and share thoughts on this?

@dougbu
Copy link
Member

dougbu commented Aug 14, 2017

type="datetime-local" is incomplete for scenarios involving systems that are in potentially-different time zones. Browsers and the W3C have deprecated the complete choice (type="datetime").

Options include:

  1. The W3C School's recommendation is to include a separate field allowing users to specify the timezone of their choice. This solution should probably be about scaffolding and not MVC itself.
  2. MVC could however expand <input asp-for="{some DateTimeOffset expression}"/> into two elements. One would be the requested <input/> (of type datetime-local) and the other a <select> containing well-known timezones. This would be a breaking change and it would be difficult to style the generated elements. For example, should all attributes in the source <input/> be applied to the <select>? Also, how would less-frequently-used timezones be chosen?
  3. Another choice would be to determine the browser's timezone from request headers. Then, model bind using the correct offset and generate RFC3339-compliant strings for that timezone. I'm not sure this information is consistently available…
  4. Finally, could mostly punt the issue and use type="text" with one of the round-tripping formats e.g. 0:yyyy-MM-ddTHH:mm:ss.fffK instead of auto-selecting datetime-local. type="datetime-local" would still be supported when chosen in the .cshtml. One downside here is end users must be cognizant of their timezone as an offset from UTC. Another is correctly-formatted values aren't particularly usable. (That said, many desktop browsers treat type="datetime-local" as if it were text anyhow.)

@DamianEdwards
Copy link
Member Author

Thanks Doug, but given that 'DateTimeOffset' is the generally recommended way to store dates in .NET now, I think we need to think more about a first-class way to support this better. It isn't always about the time-zone the user is in, but just the general proper handling of the data being bound. In this case, you suffer data loss which is terrible. We need to to mitigate that somehow, e.g. render a hidden field with original offset and have the model binder use it in connection with the value in text box when binding. Ideally we need a solution for user selection of the offset too though.

@dougbu
Copy link
Member

dougbu commented Aug 14, 2017

@DamianEdwards your "data loss" is my "incomplete" 😺 (I didn't mention the user's timezone except in 3️⃣)

To clarify, data loss occurs today in two ways: Users can't see what timezone a value is in and model binding can't infer what timezone a user meant. About all that works semi-cleanly is round-tripping when the user does not make a change. I agree a separate field (option 2️⃣ perhaps with an added hidden field or tag helper attributes to control the generated field) avoids the data loss in both directions.

Is there another generally-accepted approach I've missed?

Separately, should MVC generate something different when the user explicitly chooses type="datetime-local" versus when that type is defaulted based on the expression?

@DamianEdwards
Copy link
Member Author

Data is lost when the original value has an offset that is different to the server's system locale. That should be fixed as it's clearly a bug in our handling of this end-to-end.

First class support for offset selection is a more advanced matter that we should investigate.

@dougbu
Copy link
Member

dougbu commented Aug 14, 2017

Data is lost when the original value has an offset that is different to the server's system locale.

Yes, the chosen RFC3339 format (yyyy-MM-ddTHH:mm:ss.fff) simply truncates the offset. That breaks even the round-tripping case because the implicit assumption is the offset corresponds to the server's current timezone. Good news is this happens less often because .NET parses even values like 2017/10/01T12:35+7:00 into a DateTimeOffset with the current offset e.g. 2017-09-30T22:35:00.000-07:00 where I'm at.

I'm unsure about offering a partial solution e.g. just providing the hidden field only helps with round-tripping data the server provided.

@DamianEdwards
Copy link
Member Author

We can talk in person then. It's simply broken for cross-timezone scenarios, and we have no guidance on allowing selection of time-zone on top of that. We need to do something here.

@dougbu
Copy link
Member

dougbu commented Aug 28, 2017

Looks like this is actually a regression in ASP.NET Core 2.0.0. We currently run in RFC 3339-compliant mode and render DataTimeOffset properties using type="datetime-local" by default. It should take both changes to lose the offset and neither behaviour was the default in MVC 5.x.

This appears to also be a regression from ASP.NET Core 1.1.0 behaviour. We switched to be RFC 3339-compliant by default in in 1.0.0-rc2 (see issue #4102, PR #4250 and 6b369ef). But, the change to use type="datetime-local" didn't happen until 2.0.0-preview1 (see issue #4871, PR #5664 and f95d49c).

I'll test ASP.NET Core 1.1 to confirm the defaults round-trip the offset later today or tomorrow…

@dougbu
Copy link
Member

dougbu commented Aug 29, 2017

Have confirmed problem does /not/ exist in ASP.NET Core 1.1.0.

@dougbu
Copy link
Member

dougbu commented Aug 29, 2017

Possible approaches:

  1. Stop using type="datetime-local" for DateTimeOffset values i.e. undo part of the breaking change we took in 2.0.0 (Input tag helper for DateTime should use "datetime-local" instead of deprecated "datetime" #4871 et cetera). This will mean DateTimeOffset values are shown as text fields containing the full value. (Might choose to use type="text" to make that explicit since few if any current browsers support type="datetime".) This is both a breaking change and an unbreak of the 2.0.0 regression.
  2. Add a type="hidden" field containing the Offset value for DateTimeOffset values used in <input/> tag helpers and Html.Editor[For](...). This would also require a DateTimeOffset model binder which combines the two fields. Unfortunately, this may break some with a workaround for Input fields don't emit (and thus round-trip) offset value when binding against DateTimeOffset fields #6648 already in place. This approach also fixes only part of the problem: Unless the offset or associated timezone is visible in the form, users are very likely to submit values that are misinterpreted.
  3. Implement 1. and document the heck out of possible issues when using DateTimeOffset with type="datetime-local" in an ASP.NET Core application. Since type="datetime-local" will remain available (as an explicit choice), .cshtml authors can create forms which consistently make the offset obvious to their users.
  4. Implement 4. plus the model binder mentioned in Don't use Owin in the names (like IOwinRequest owinRequest) #2. This approach avoids the default data loss on user submission but allows .cshtml authors to easily create forms that allow their users to choose offsets.

If we're talking about taking this in 2.0.1, I recommend doing 1. initially. That's an E2E fix which involves very little code.


I'm probably leaving out another half dozen approaches. Please speak up, especially if you have an idea we could implement in a patch.

@DamianEdwards
Copy link
Member Author

I agree on fixing this via option 1 in a servicing release. We may need to offer a quirk switch to enable opting in to the 2.0 RTM behavior given the breaking nature of the fix.

Beyond that I don't care to think too much about right now other than I think we should do a much better job of supporting DateTimeOffset via features or guidance.

@JamesNK
Copy link
Member

JamesNK commented Aug 30, 2017

You should audit how well ASP.NET Core handles DateTimeKind at the same time.

Converting to and from ISO date strings and DateTime(local,utc,unspecified) + DateTimeOffset is surprisingly complex.

@mattjohnsonpint
Copy link

Allow me to offer some guidance. First, to clear up a few points:

  • <input type="datetime-local" /> is specifically for date and time without an offset. RFC 3339 specifically requires an offset. Therefore, they are incompatible.
  • RFC 3339 is a profile of the ISO 8601 standard. There are other profiles in the standard, including one that is date and time without an offset. Therefore, if we truncate the offset, we are not RFC 3339 compliant, but we are still ISO 8601 compliant. However, that profile should be used with a DateTime with .Kind == DateTimeKind.Unspecified only. Using it for a DateTimeOffset, or a DateTime with Local or Utc kind would be in error.
  • Time zone information is not present in HTTP Request headers, so the idea of model-binding to that is out. Additionally, I'll echo @DamianEdwards in that DateTimeOffset values are regularly passed with offsets other than the browser's or the server's.
  • Locale and time zone are orthogonal concepts. I think instead of locale, you meant the server's local time zone setting. However, this should be considered irrelevant, as it's a system-wide setting. One can set the default locale of a specific application, but one cannot set the local time zone of an application without potentially affecting all other programs on the server.

Regarding presenting the user with a drop-down lists of time zones, I'll point out that the example @dougbu linked to on Mozilla's site is flawed. Time zone names can only be mapped to identifiers such as those used with TimeZoneInfo.Id (either Windows or IANA representations depending on runtime platform). Names should not be assigned to numeric offsets, as that would be a gross misrepresentation. For more on this, see "Time Zone != Offset", from the timezone tag wiki on StackOverflow

The ideal mapping for DateTimeOffset would have been <input type="datetime" />, which no longer exists in the HTML spec. Using <input type="text" /> would work, but it requires the developer to bring their own date/time picker controls, manage formatting, validation, etc.

My recommendation is to use <input type="datetime-local" /> and a <select /> drop-down list of offsets.

Offsets are interesting. To be future-proof and spec-compliant with RFC 3339 and ISO 8601, they would have to support any valid ±HH:mm from -23:59 to +23:59 and every minute in between. However a reduced list is indeed possible. We only support offsets from -14:00 to +14:00 on DateTimeOffset anyway, so hopefully Kiritimati never implements daylight saving time. ;)

I've written some code in this dotnetfiddle that shows how to build a list of offsets, both in "simplified" form that shows just the current offsets used throughout the world (covering both standard and daylight time), and how to build an expanded form showing all representable offsets that a DateTimeOffset value might use. I recommend making that an option the user can set via attribute, with simplified being the default.

Of course, one might want to generate the full list via JavaScript to save on bandwidth - but that's not currently possible with the simplified form without pulling in JS libraries.

@mattjohnsonpint
Copy link

By the way, a similar drop-down list is already in use as a common control in the Azure Portal. For example, try to create a scheduled web job, and you'll notice the schedule asks for a Date, Time, and UTC Offset. The offsets in the list are created from code similar to what I gave in my fiddle above.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2017

The points @JamesNK and @mj1856 raised are important especially for our long-term plans and the guidance we eventually provide.

With respect to #6648 aka our short-term fixes, our plan was basically to stop auto-selecting type="datetime-local" (and the associated format) for DateTimeOffset values. As @mj1856 makes clear, we should at least include DateTime values with Kind==DateTimeKind.Utc in that change. This will expand the breaking change we're discussing i.e. MVC will generate different HTML and lose use of browsers' datetime wizards in the default case.

Going beyond the above, I'd appreciate input on:

  • Is the distinction between Kind==DateTimeKind.Local and Kind==DateTimeKind.Unspecified important enough to further expand the above breaking change? DateTime values originally with these Kinds that lose that Kind after a round trip still describe the original point in time. And, choosing type="datetime-local" only for Kind==DateTimeKind.Unspecified values affects loads more people. (DateTimeKind.Local seems very common.)
  • Can we help people who have explicitly chosen type="datetime-local" in the short term? Users with e.g. <input asp-for="ADateTimeOffset" type="datetime-local" /> or Html.EditorFor(m => m.ADateTimeOffset, templateName: "datetime-local") in their .cshtml files may also be at corruption risk.
  • Similarly, can we help people who have explicitly chosen type="time" in the short term? That type choice also by default uses a format string which leaves out Kind and Offset information. It's good this must be explicitly requested. But, …
  • Do any browsers still support (i.e. use a wizard for) type="datetime" but truncate the offset MVC provides when submitting form data? I'm probably borrowing trouble here but would like to know.

A couple of quibbles:

  • MVC makes no claim of RFC 3339 compliance or at least shouldn't claim such compliance. Instead the software (since at least ASP.NET MVC 5.2.3) has a mode named after the RFC but only uses the full format (aka the date-time production) for type="datetime" fields. Other formats selected in this mode correspond to the full-date and partial-time productions as well as the format used for type="datetime-local": full-date "T" partial-time. MVC will only auto-select the type="datetime-local" format; the rest must be explicitly requested. Fortunately, MVC 5.2.3 and ASP.NET Core versions prior to 2.0.0 auto-selected type="datetime" in the same cases.
  • Unless in the (misnamed) RFC 3339 mode, MVC doesn't auto-select format strings at all -- just the generated type attribute. Unfortunately, that mode has been the default since ASP.NET Core 1.0.0.
  • MVC does no conversions between DateTime and DateTimeOffset values. The framework does however convert both to and from strings.

@JamesNK
Copy link
Member

JamesNK commented Aug 30, 2017

This is what Json.NET does with DateTimeKind (and DataContractSerializer which I largely followed) when parsing ISO dates.

2017-08-30T20:45:13+00:00 = Local
2017-08-30T20:45:13Z = UTC
2017-08-30T20:45:13 = Unspecified

And vice-versa when writing. You then need to worry about converting each of those to DateTimeOffset when parsing. Writing DateTimeOffset is simpler: always include the offset with the ISO date.

DateTimeOffset = 2017-08-30T20:45:13+00:00

@mattjohnsonpint
Copy link

mattjohnsonpint commented Aug 30, 2017

Thanks @JamesNK - Yes, that's the expected behavior. Though worth noting that if an offset is present and thus a DateTime is assigned DateTimeKind.Local, the date and time value must be adjusted accordingly. In the given example, if my local time zone is Pacific, then 2017-08-30T20:45:13+00:00 becomes 2017-08-30T13:45:13-07:00, which becomes 2017-08-30T13:45:13 with DateTimeKind.Local. I think this is already happening in Json.NET. It's slightly screwy - but thus is the nature of DateTimeKind.

@dougbu - further response forthcoming. Thanks.

@dougbu
Copy link
Member

dougbu commented Sep 1, 2017

It turns out f95d49c was stronger than I remembered and a few potential workarounds e.g. Html.EditorFor(m => m.StartTime, templateName: "DateTime") won't help. I'll list what /will/ work prior to a #6648 fix after some testing…

@mattjohnsonpint
Copy link

I did some testing. Presently with a DateTime value in the model, we create <input type="datetime-local" />. Regardless of Kind of the value in the model, we treat it as if it were DateTimeKind.Unspecified, such that a value compatible datetime-local is used. No adjustment of value is made. In other words, just 2017-09-01T00:00:00.000, even if the kind was Local or Utc. Basically, Kind doesn't round-trip.

(Also, the docs need updating to show datetime-local instead of datetime.)

If we wanted to be able to round-trip DateTimeKind, similar to what Json.NET is doing, we'd have to persist the kind somewhere, such as an associated hidden input field. That said, I don't think DateTimeKind round-tripping is that big of a deal. Users are somewhat used to this, going back to the early days of ASP.NET, for both WebForms and MVC. If you ask a user for date and time input, the general expectation is that the value you receive is DateTimeKind.Unspecified. If you meant for that to be interpreted as UTC time, then you can use DateTime.SpecifyKind to set it explicitly. Similarly, if you meant it to be interpreted as time in a specific time zone, then TimeZoneInfo.ConvertTime would be employed. Let the user continue to do that for DateTime.

For DateTimeOffset, there's indeed work to do. However, it would only matter if a DateTimeOffset was in the UI model in the first place. There are indeed scenarios for picking a date, time, and offset in user input, but they are much less common than picking a date or a date and time. I still think the best approach for this is <input type="datetime-local"/><select /> with the select list being populated by at least the simplified offsets (per my earlier example). Of course, I'm making an assumption that the concerns @dougbu raised about styling two controls could be resolved.

All of the above and current implementations leave <input type="date" /> and <input type="time" /> unaddressed, and there are indeed many use cases for both date-only (ex: date-of-birth) and time-only (ex: business hours) entry in HTML forms. (Otherwise they wouldn't be in HTML5.) To address these, could we please promote the System.Time package I contributed two years ago out of corefxlab? That code introduces a System.Date type that would cleanly map to <input type="date" />, and a System.TimeOfDay type that would cleanly map to <input type="time" />.

To answer some questions posed:

Is the distinction between Kind==DateTimeKind.Local and Kind==DateTimeKind.Unspecified important enough to further expand the above breaking change?

It is quite a distinction, in that Local only represents the machine where the code it's running. This should be discouraged in web applications, as the server's time zone is generally irrelevant, and is a global setting for all apps on the machine. Really, any DateTimeKind.Local usage should be avoided in ASP.NET. I am working on addressing guidance for this in dotnet/AspNetCore.Docs#3992.

DateTime values originally with these Kinds that lose that Kind after a round trip still describe the original point in time.

No, they don't. They have the same date and time values, but when Kind is Unspecified, there is no point in time being represented. It's a civil date (as on a calendar), and a time of day (as on a wall-clock). The context of how that relates to a point on the universal timeline is lost. However, like I said, that's probably ok here - as that's how it's always been and there are workarounds.

And, choosing type="datetime-local" only for Kind==DateTimeKind.Unspecified values affects loads more people. (DateTimeKind.Local seems very common.)

I think Local is only common due to developers reaching for DateTime.Now. As mentioned above, not a good idea in a web app. Unspecified is the default when loading DateTime values from a SQL Server (with or without EF), and is the default when just using the constructor new DateTime(y, m, d, ...). So it's just as common, IMHO.

I think the confusion around the word "local" here is part of the problem. HTML5 is borrowing terminology from ISO8601 (as do Noda Time, Java 8, and others). In that context, "local" just means "local to someone somewhere", where in .NET local means "on the local machine where the code is executing". (BTW, there's a bike-shed for this in tc39/proposal-temporal#33 if you're interested).

Really, DatetimeKind.Unspecified is the only correct alignment for <input type="datetime-local" />.

Can we help people who have explicitly chosen type="datetime-local" in the short term?

We can warn them in documentation, but I cannot think of any technical way to resolve this other than that already described. Round-tripping the offset in a hidden field doesn't seem like a good option, as it doesn't allow the user to adjust it. (ex: Pacific is now -07:00 (PDT), but maybe -08:00 (PST) for a different date).

Similarly, can we help people who have explicitly chosen type="time" in the short term?

YES - by promoting the System.Time package out of corefxlab as mentioned above, and suggesting users adopt it for date-only and time-only values.

Do any browsers still support (i.e. use a wizard for) type="datetime" but truncate the offset MVC provides when submitting form data?

No, according to this chart, and still limited support for ``type="datetime-local"`. I tested myself on Chrome, Edge, FF, and IE (all latest on Win10) and found the same results.

MVC does no conversions between DateTime and DateTimeOffset values.

Sure, but .NET does have a one-way implicit cast from DateTime to DateTimeOffset. It uses the Kind to set the offset. It gets folks into trouble every so often (from my own experiences and on StackOverflow).

@dougbu
Copy link
Member

dougbu commented Sep 2, 2017

Thanks very much @mj1856. <input type="datetime-local" .../><select ...>...</select> is certainly the right way forward in our guidance if not our helpers.

However, I'd like to focus this issue on what we can do about losing Offsets and possibly Kinds in our next 2.0 patch. Updating the helpers would be too large a breaking change for a patch e.g. the change would ruin HTML layout. It would also be an unusually large amount of new code. Once we decide on guidance and product changes or some mix for the future, I'll open the appropriate MVC 2.x and Docs issues.

@dougbu:

Is the distinction between Kind==DateTimeKind.Local and Kind==DateTimeKind.Unspecified important enough to further expand the above breaking change?

@mj1856:

I don't think DateTimeKind round-tripping is that big of a deal

...

It is quite a distinction,

...

DatetimeKind.Unspecified is the only correct alignment for <input type="datetime-local" />

What is the bottom line here? Should MVC's next patch change how <input asp-for="ADateTime"/> and Html.EditorFor(m => m.ADateTime) work or not? (Changing things for ADateTimeOffset is a given.)

could we please promote the System.Time package I contributed two years ago out of corefxlab?

Seems like a good idea. But, what does "promote" mean in this context?

@mattjohnsonpint
Copy link

mattjohnsonpint commented Sep 2, 2017

For DateTime

  • The MVC behavior isn't ideal because DateTimeKind is not ideal.
  • Since there are workarounds (using DateTime.SpecifyKind after binding), I recommend leaving this as currently implemented. All DateTime types should produce an <input type="datetime-local" />.
  • AFAIK, Kind has never fully round tripped in an MVC control, so I don't see this as anything urgent.
  • The documentation should be updated to reflect datetime-local instead of datetime, and perhaps mention the workaround in a note block. I can send a PR for both if you like.

For DateTimeOffset

  • We agree on the two fields for the long-term solution, with the key bit being the select is a list of offsets, not a list of time zones.
  • For the short-term I recommend switching this to <input type="text" />. It is a better option than <input type="datetime-local" /> since it wont lose offset. It doesn't present a UI, but neither did the previous <input type="datetime" /> since it never fully came to be.
  • It's worth noting that DateTimeOffset may not be the best option for the use case @DamianEdwards identified - at least not for the UI model. Typically, having DateTime fields and a separate time zone selection works best. The application logic can then compute the DateTimeOffset values, rather than asking the user to pick the offset. This is most likely what end users will do, and aligns with other guidance I give with regard to scheduling.

For Date and TimeOfDay (System.Time package)

  • I'm not sure what the process is for promotion from corefxlab, but I would expect it to go through some sort of scrutiny and ultimately land in corefx.
  • Currently, it is only available in the corefxlab myget feed. This would make it available as a public Nuget package.
  • At that point, ASP.NET could map Date to <input type="date" /> and TimeOfDay to <input type="time" />. This completes support for the temporal types allowed in the HTML5 spec.
  • This seems like a good fit for ASP.NET Core 2.1 on the roadmap.

@dougbu
Copy link
Member

dougbu commented Sep 2, 2017

Going back to workarounds:

  1. If the UI can change significantly, use <input type="datetime-local" .../><select ...>...</select> or a similar HTML helper combination, populating the drop-down as @mj1856 described above. This is the only option that maintains pickers for DateTimeOffset values in modern browsers.
  2. If the UI cannot change that much and the .cshtml files use Html.Editor(...) or Html.EditorFor(...) with DateTimeOffset expressions, add an editor template for DateTimeOffset e.g.
  @model DateTimeOffset
  <input asp-for="@Model" type="datetime" class="form-control" />
  1. If the .cshtml files use <input/> tag helpers with DateTimeOffset expressions, change all of them to include type="datetime". Browsers will ignore the type (treating it as type="text") but MVC will choose a format string that round-trips the Offset.
  2. To more explicitly handle the 3. case, change the relevant tag helpers to include type="text" asp-format="{0:o}". A custom format string including "K" or "zzz" also works.
  3. If it's easier to handle the 2. and 3. cases in the model, add annotations to DateTimeOffset properties e.g.
  [DisplayFormat(ApplyFormatInEditMode = true, DataFormatString = "{0:o}")]
  [UIHint("text")]
  1. To handle the 2. and 3. cases centrally, add an IDisplayMetadataProvider implementation in the Startup class e.g.
  services.AddMvc(options => options.ModelMetadataDetailsProviders.Add(new DatetimeMetadataProvider()));
...

  private class DatetimeMetadataProvider : IDisplayMetadataProvider
  {
      public void CreateDisplayMetadata(DisplayMetadataProviderContext context)
      {
          if (context.Key.ModelType == typeof(DateTimeOffset) ||
              context.Key.ModelType == typeof(DateTimeOffset?))
          {
              // If model type hasn't been annotated as we'd like...
              var metadata = context.DisplayMetadata;
              if (string.IsNullOrEmpty(metadata.EditFormatString) &&
                  string.IsNullOrEmpty(metadata.DataTypeName) &&
                  string.IsNullOrEmpty(metadata.TemplateHint))
              {
                  // Fix things up. TemplateHint = "DateTime" does not help. Changing EditFormatString is not
                  // required when using DateTimeOffset. Then again, the default DateTimeOffset format in most
                  // locales loses sub-Second information.
                  metadata.EditFormatString = "{0:o}";
                  metadata.TemplateHint = "text";
              }
          }
      }
  }

The above choices also work for DateTime values if your application needs to round-trip the Kind.

@mattjohnsonpint
Copy link

@dougbu - I agree with most of that. Note that K format specifier is for DateTime and zzz specifier is for DateTimeOffset. Don't mix them, since zzz on a DateTime doesn't properly reflect the Kind but rather the OS time zone.

I do think it's super important that datetime-local be preserved for use cases of just picking a date and time with a DateTime, and the expectation that Kind == DateTimeKind.Unspecified is reasonable. Round-tripping the kind in this particular case is not essential. However, if it's possible to add a parameter for the tag helper that would allow for specifying Kind to set, that would be useful.

@dougbu
Copy link
Member

dougbu commented Sep 2, 2017

I really appreciate your suggestions @mj1856!

The documentation should be updated to reflect datetime-local instead of datetime, and perhaps mention the workaround in a note block. I can send a PR for both if you like.

This seems like cruft in the documentation and I agree we should improve it. Suggest we also mention:

  • MVC 5.x (yeah, a historical reference 😈) and ASP.NET Core 1.x do use type="datetime" for DateTime and DateTimeOffset values. This amounts to type="text" except for the format string MVC auto-selects.
  • The ASP.NET Core 2.0.x patch may use type="datetime" again if that temporary fix is simpler.

A PR would certainly help. @scottaddie? @Rick-Anderson?

@dougbu
Copy link
Member

dougbu commented Sep 2, 2017

I do think it's super important that datetime-local be preserved

Agreed. I'm not going to make any changes undoing a developer's explicit selection of this type. This issue is limited to MVC's auto-selection based on the datatypes.

@mattjohnsonpint
Copy link

mattjohnsonpint commented Sep 2, 2017

Right, I mean that datetime-local is currently the auto-selected choice for the DateTime type, and I think it should remain so.

@dougbu
Copy link
Member

dougbu commented Sep 2, 2017

FYI I edited my earlier comment because using type="text" is actually quite simple.

The ASP.NET Core 2.0.x patch may use type="datetime" again if that temporary fix is simpler.

dougbu added a commit that referenced this issue Sep 3, 2017
- #6648
- a different take on #4871
- `DateTime` can round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]`
- since they're now handled differently by default, add more `DateTime` tests
- expand tests involving `Html5DateRenderingMode.CurrentCulture`
dougbu added a commit that referenced this issue Sep 5, 2017
- #6648
- a different take on #4871
- `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]`
- since they're now handled differently by default, add more `DateTime` tests
- expand tests involving `Html5DateRenderingMode.CurrentCulture`

nits: make VS-suggested changes to files updated in this PR
@dougbu
Copy link
Member

dougbu commented Sep 5, 2017

7e4a8fe

@dougbu dougbu closed this as completed Sep 5, 2017
@scottaddie
Copy link
Member

@mj1856 Please let me know if you plan to submit a docs PR for this; otherwise, I'll add it to my list of things to do.

@mattjohnsonpint
Copy link

@scottaddie - Yep, will do shortly. Thanks.

dougbu added a commit that referenced this issue Sep 17, 2017
- cherry-picked from 7e4a8fe in dev
- #6648
- a different take on #4871
- `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]`
- since they're now handled differently by default, add more `DateTime` tests
- expand tests involving `Html5DateRenderingMode.CurrentCulture`

nits: make VS-suggested changes to files updated in this PR
dougbu added a commit that referenced this issue Sep 17, 2017
…quirks mode

- patch recipients can use switch to undo the #6648 fix
dougbu added a commit that referenced this issue Sep 18, 2017
- cherry-picked from 7e4a8fe in dev
- #6648
- a different take on #4871
- `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]`
- since they're now handled differently by default, add more `DateTime` tests
- expand tests involving `Html5DateRenderingMode.CurrentCulture`

nits: make VS-suggested changes to files updated in this PR
dougbu added a commit that referenced this issue Sep 18, 2017
…quirks mode

- patch recipients can use switch to undo the #6648 fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants