Skip to content

Conversation

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented May 26, 2022

Fix time/datetime-local binding with seconds when there's no step attribute

Restores the ability to do <input type=time @bind="someDateTimeVariable"> without getting a weird browser warning if someDateTimeVariable has a non-zero "seconds" component. This used to work in 5.0 but regressed in 6.0.

Description

In 5.0, there was explicit JS-side logic to handle the case where an <input type=time> had no step attribute (which browsers interpret as "use whole number of minutes - no seconds"). We automatically stripped off any "seconds" part of the formatted string to make it valid as far as the <input type=time> is concerned. This behavior was unintentionally lost in 6.0.

Fixes #41731

Customer Impact

Without this change:

  • People upgrading 5.0 applications to 6.0
  • Or, people writing new 6.0 applications without realising they would need to ensure bound DateTime or TimeOnly values have a zero "seconds" component when used with <input type=time> or <input type=datetime-local>

... would find that:

  1. The UI displays a weird tooltip saying that the time value is invalid
  2. The values retrieved from an <input type=time> or <input type=datetime-local> would have a nonzero "seconds" component only if they have not been edited by the user, which is pretty odd

Regression?

  • Yes
  • No

Worked in 5.0

Risk

  • High
  • Medium
  • Low

It is plausible that a 6.0 developer has taken a dependency on this weird behavior. For example, they might be using the <input type=time> as a readonly value, and be relying on the ability to round-trip a value that includes a nonzero "seconds" part. However:

  1. This is a bad UX, as the browser will be showing a tooltip saying the value is invalid
  2. This would be a very flaky pattern to follow, because "input" elements are primarily intended for editing, and the moment the user edits the value, the "seconds" part would be reset to zero

The 5.0 behavior is objectively better, so I have no doubt we should fix this in 7.0. Whether or not we should also patch 6.0 and accept the risk that some developer actually wants the bug is a more ambiguous question.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner May 26, 2022 12:33
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label May 26, 2022
@ghost ghost added this to the 6.0.x milestone May 26, 2022
@ghost
Copy link

ghost commented May 26, 2022

Hi @SteveSandersonMS. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@SteveSandersonMS SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label May 26, 2022
@ghost
Copy link

ghost commented May 26, 2022

Hi @SteveSandersonMS. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@SteveSandersonMS
Copy link
Member Author

cc @mkArtakMSFT

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels May 26, 2022
@SteveSandersonMS
Copy link
Member Author

@mkArtakMSFT This can be merged

@mkArtakMSFT
Copy link
Contributor

@wtgodbe when will the branches be ready to merge this? Can I merge this now?

@wtgodbe
Copy link
Member

wtgodbe commented May 31, 2022

Branches will open after we do branding next Tuesday (6/7)

@dougbu dougbu merged commit 0d68c8f into release/6.0 Jun 7, 2022
@dougbu dougbu deleted the stevesa/backport-41868 branch June 7, 2022 23:49
@dougbu dougbu modified the milestones: 6.0.x, 6.0.7 Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants