Fix due dates not rendering with a one-off date

[yardenshoham]

• Fix MIlestones with 2/29 due date display as 2/28 #29034

Some timestamps are not meant to be localized in the user's timezone, only in their language. This is true for due date timestamps. These include a specific year, month, and day combination. If a user in one timezone sets the date YYYY-MM-DD, another user should see the same date, regardless of their timezone. So when a relative-time element has their datetime attribute specified only as YYYY-MM-DD, we will update it to YYYY-MM-DD midnight in the user's timezone. When the date is rendered, the only localization that will happen is the language.

For all relative-time elements, if their datetime attribute is YYYY-MM-DD, we will update it to YYYY-MM-DD midnight in the user's timezone.

Demo GIFs

In these GIFs, I do the following:

1. Create an issue
2. Set its milestone to July 1, 2024
3. Show how it's rendered (it's rendered correctly in both cases)
4. Change my timezone to Arizona
5. Refresh the page
6. Show how it's rendered (incorrect in the "before" GIF, correct in the after GIF)

Before

After

[yardenshoham]

I wish I could make the datetime replacement as early as possible, ideally before the first relative-time render but I guess a small flash is ok if it fixes the bug.

Can I change the behavior of the relative time web component? Maybe go with a mutation observer?

[silverwind]

Can I change the behavior of the relative time web component? Maybe go with a mutation observer?

Not directly, unless we fork it. Ideally you would contribute your fix upstream if it is deemed an acceptable change there.

We could wrap the custom element an another custom element of ours, but this will be a very complex effort I assume. MutationObserver would likely be more straightforward but care needs to be taken so won't end up creating infinite loops.

[silverwind]

I think #29034 (comment) would be a much better solution than to wrestle with this in the frontend.

[yardenshoham]

How would #29034 (comment) solve this? Today we already send only the date without the time...

[yardenshoham]

We could wrap the custom element an another custom element of ours, but this will be a very complex effort I assume. MutationObserver would likely be more straightforward but care needs to be taken so won't end up creating infinite loops.

I tried. I couldn't make it work. Let's go ahead and land as-is to fix the bug as soon as possible.

[silverwind]

How would #29034 (comment) solve this? Today we already send only the date without the time...

Do we? Then the value is interpreted incorrectly on the frontend. A date must be interpreted as a date, not a datetime.

I tried. I couldn't make it work. Let's go ahead and land as-is to fix the bug as soon as possible.

Sorry, but this requires deeper investigation on why we interpret date as datetime. Does <relative-time> support passing only a date without time?

[yardenshoham]

Yes, <relative-time> supports passing only a date without time, this is what we do today. But it takes the given date with the assumption it's 00:00:00 UTC. That's what's causing the bug.

[silverwind]

But it takes the given date with the assumption it's 00:00:00 UTC

That's where it goes wrong. There simply is no time component to a date, interpreting as 00:00 is just wrong. Maybe we should just remove <relative-time> from date rendering completely if it can't do what we need?

I could easily come up with a webcomponent that accepts a date in a attribute and renders it in the document's locale.

[yardenshoham]

But we need it for the language localization

[silverwind]

Example of such a component:

function toLocaleDate(date) {
  return date.toLocaleString(document.documentElement.lang, {year: 'numeric', month: 'long', day: 'numeric'});
}

window.customElements.define('locale-date', class extends HTMLElement {
  connectedCallback() {
    this.textContent = toLocaleDate(this.getAttribute('data-date'));
  }
});

[silverwind]

I'm not completely sure above is right because it goes through JS Date which does have the time component, but I'm sure such quirks can be worked out easily via unit tests.

[yardenshoham]

I don't think we should have 2 different components to render localized dates

[silverwind]

Well it entirely depends whether <relative-time> can support dates without time properly, or not. If not, it is unsuitable for date rendering.

[yardenshoham]

Right, but this PR fixes it.

[silverwind]

I can't accept such hacks without reading into <relative-time> docs to check whether we are maybe just using it incorrectly, I suggest you do too :)

[silverwind]

Checking GitHub's milestone due date rendering, they are also not using <relative-time> there, likely for good reason:

[yardenshoham]

Yeah but they only render English, never other formats

[silverwind]

Yes, that's why we need #29698 (comment).

[silverwind]

So I read through https://github.com/github/relative-time-element?tab=readme-ov-file#relative-time-element and I'm convinced it's just not meant for date rendering. It accepts only datetime, if it woud support date it would accept date.

I can likely create the webcomponent in another PR if you are unwilling to do, I strongly believe it's the right way and we should not abuse <relative-time> for things it's not meant to render.

[yardenshoham]

I'd rather not create the component myself, go ahead and create a PR for that. Maybe until you do though we could merge this one

[silverwind]

Where does the backend currently render these dates? Is it here?

https://github.com/go-gitea/gitea/blob/main/modules/timeutil/datetime.go

[yardenshoham]

gitea/templates/repo/issue/view_content/sidebar.tmpl

Line 367 in 7f856d5

{{DateTime "long" .Issue.DeadlineUnix.FormatDate}}

gitea/routers/web/repo/issue.go

Line 2312 in 7f856d5

deadline = time.Date(form.Deadline.Year(), form.Deadline.Month(), form.Deadline.Day(),

[silverwind]

Thanks, so for rendering we want to only use deadline (time.Date) and for comparison, deadlineUnix. E.g. a deadline is exceeded when it hits midnight servertime, it does not make sense to convert this to any user timezone.

[yardenshoham]

Yeah, this PR reverts the automatic timezone conversion done by <relative-time>

[silverwind]

So I tried implementing like above but the implementation suffers the same time zone problem. The root of the problem is that when passing a yyyy-mm-dd format to Date, it assumes timezone offset is UTC, so one would have to add the negative timezone offset to it to land at midnight current timezone.

So I think this workaround is acceptable until we have Temporal.PlainDate in a few years or so 😆, which will cleanly solve it. Maybe date support could also be contributed to <relative-time>, but it likely won't be easy.

[silverwind]

On the other hand, this solution has a almost unacceptable flash of incorrect content because it loads as part of the main JS bundle.

[yardenshoham]

almost

[silverwind]

#29725

[silverwind]

Can't get rid of that content flash, so I just added this simple negative offset calculation to my component and now it works flawlessly.

[yardenshoham]

Yeah #29725 is better because there's no content flash