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

Migrate datetime chooser init script to Stimulus w-date #10261

Closed

Conversation

lb-
Copy link
Member

@lb- lb- commented Mar 24, 2023

  • Avoid all need for Telepath widgets for datetimepicker, remove all inline script usage, replace with a single Stimulus controller 'w-date'
  • Refines 'soft' focus approach (which just did not focus on datetimepicker at all) with a DOM event / preventDefault approach so that we can use similar functionality without needing the overhead of a Telepath instance for just this behaviour. Add unit tests for this new Telepath widget behaviour
  • Ensure that Block variants use the same HTML (which will now include autocomplete off)
  • Ensure that options cannot override core functionality (hideCurrent, change event dispatch)
  • Split out date compare into own util (with tests)
  • Add unit test functionality to cover all datetimepicker usage scenarios
  • Remove the need for a dedicated datetimepicker JS import
  • Support existing wagtailConfig global usage
  • Fix console error when running widget unit tests (global scrollTo not provided by JSDom)
  • Find another test case to check the media inheritance approach as no longer needed for Admin date widgets
  • Fixes 🎛️ Migrate datetime picker widgets to a Stimulus Controller w-date #10260

Implementation & testing notes

  • Tested on Safari 16 (a very small flash of the widget shows in Safari), Firefox 101, Chrome 111. Tested with macOS voiceover on all browsers.
  • The soft focus approach works better than before with making the actual date picker the 'next' keyboard focusable target and correctly reads the context of the container to screen readers.

@squash-labs
Copy link

squash-labs bot commented Mar 24, 2023

Manage this branch in Squash

Test this branch here: https://lb-feature10260-stimulus-migra-cyycu.squash.io

@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from bed4a5e to c4f93bc Compare March 24, 2023 21:50
@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from c80a8f3 to 15581a6 Compare March 25, 2023 11:50
@lb- lb- marked this pull request as ready for review March 25, 2023 11:50
@lb-
Copy link
Member Author

lb- commented Mar 25, 2023

This is now ready for review.

@lb- lb- added status:Needs Review Accessibility javascript Pull requests that update Javascript code labels Mar 25, 2023
@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from 15581a6 to 0f14d60 Compare March 25, 2023 12:09
@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from 0f14d60 to 1cf3705 Compare April 21, 2023 22:08
@lb-
Copy link
Member Author

lb- commented Apr 21, 2023

Rebased with upgrade considerations moved to 5.1 release notes.

@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch 2 times, most recently from a96f8ed to c8a36ad Compare April 22, 2023 05:35
@lb-
Copy link
Member Author

lb- commented Apr 22, 2023

Fixed up the additional failing tests - should be good now.

@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from c8a36ad to 36e4cc2 Compare May 29, 2023 21:46
@lb-
Copy link
Member Author

lb- commented May 29, 2023

Rebased

@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch 2 times, most recently from 87752a3 to 07aa418 Compare June 11, 2023 10:05
@lb-
Copy link
Member Author

lb- commented Jun 11, 2023

This has not had a review yet but I have been thinking about the previous approach and found a much simpler solution for the 'soft' focus for use in the date picker. Previous approach can be found in my branch 10260-stimulus-migration-datetime-picker-with-focus-override

This now avoids any <100ms flash of the datepicker being open in Safari and overall is a cleaner solution in the code.

I also reverted the removal of the soft focus call as this is a documented approach. I have opted to NOT document the new Custom Event dispatching as it will be for default widgets only and is not something that can be easily implemented at the top level usage of calling focus.

Would love to see this get a review when someone has a chance, I feel this is a more accessible/secure, simpler and hopefully more maintainable solution. Of course happy to take feedback and push back on the implementation.

client/src/utils/date.ts Show resolved Hide resolved
client/src/controllers/DateController.ts Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
docs/releases/5.1.md Show resolved Hide resolved
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lb-, thank you for putting this together! I had a chat with Thibaud (and if I remember correctly the accessibility subteam had discussed this as well), we're keen on migrating the date/time pickers to the browser's native inputs. We're not exactly sure if everything can be handled gracefully with the browser's built-in inputs (especially around timezones), so we might still need some JavaScript via Stimulus in the end. In any case, this means we won't be needing the xdsoft library, which unfortunately means we won't go ahead with this PR as-is. I hope that's OK!

@laymonage
Copy link
Member

Oops, I've cancelled my in-progress review but for some reason GitHub still posted them anyway. Not sure if you can see them, but if so, you can ignore them.

@lb-
Copy link
Member Author

lb- commented Jun 27, 2023

no problems, I knew that was coming at some but but still assumed we would want the basics in place to hook in the JS changes/reduction when it came.

I do think there would be a desire from those working with Wagtail to opt for custom date widgets and assumed we would likely want to support that in some basic way.

If we are close to implementing the new approach, sounds good. If we feel it's still a release or two away, I would like to suggest we do review this PR for merging as it is one of the largest sources of inline script tags.

Happy with whatever we do here. Feel free to close this PR if it's not something we want to merge at this time though.

@lb-
Copy link
Member Author

lb- commented Jun 30, 2023

@laymonage & @thibaudcolas - a proposal for a different tact for date pickers, would love your thoughts.

  1. move ahead with this PR, I can review and revise based on the cancelled feedback.
  2. I add a new setting, something like wagtail_legacy_date_fields, this will be set to True for 5.1 with release notes about this config, ready to change the default to False in a future release.
  3. I adjust the JS to load Async, dynamic require, only if the controller loads on a page and the legacy approach is true.

This gives us the benefits of being able to pull out critical path inline scripts, gives us time to confirm the native field approach and also paves the way for being closer to removing insert_editor_js.

We can set this config to False in bakerydemo, Guide site and maybe the Wagtail website. Plus gives some in the community to trial the approach.

If we realise we do need some JS, we have the Controller scaffold to build on. If we simply want to make it easier to BYO date widgets, we can change the controller to be a basic skeleton.

I'm probably a bit biased, having worked on this for a while, but do want to push a bit on giving us all time to fully move away from the xdsoft code but also give us a nice win for CSP, JS loading and accessibility options.

@laymonage laymonage added this to the 5.1 milestone Jul 11, 2023
@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from 07aa418 to 97ff0fb Compare July 12, 2023 11:52
@lb-
Copy link
Member Author

lb- commented Jul 12, 2023

Rebased/resolved conflicts.

lb- added 4 commits July 13, 2023 18:17
- Avoid all need for Telepath widgets for datetimepicker, remove all inline script usage, replace with a single Stimulus controller 'w-date'
- Use 'soft' focus blocking via DOM event & prevent default, enhance to actually focus on the parent element instead of nothing
- Ensure that Block variants use the same HTML (which will now include autocomplete off)
- Ensure that options cannot override core functionality (hideCurrent, change event dispatch)
- Split out date compare into own util (with tests)
- Add unit test functionality to cover all datetimepicker usage scenarios
- Remove the need for a dedicated datetimepicker JS import
- Support existing wagtailConfig global usage
- Find another test case to check the media inheritance approach as no longer needed for Admin date widgets
- Resolves wagtail#10260
@lb- lb- force-pushed the feature/10260-stimulus-migration-datetime-picker branch from 97ff0fb to 96badbf Compare July 13, 2023 08:20
@laymonage laymonage modified the milestones: 5.1, 5.2 Jul 18, 2023
@lb-
Copy link
Member Author

lb- commented Jul 18, 2023

Note: Moved to 5.2, still waiting on direction whether I close this or rebase. Will flag as needs design decision.

@laymonage
Copy link
Member

@lb- Thibaud and I have gone back and forth on this a few times, and I think we were still leaning towards completely replacing the inputs to the native browser inputs and making sure that we iron out any issues before merging, as opposed to having the old one as fallback in case there are issues with the new implementation. Even without a fallback, reviewing this PR is quite non-trivial and since we're going to replace it soon anyway, we feel it'd be better to focus our time on the new implementation instead.

It does sound a bit too optimistic to think that we'll be able to cover any issues before the change is released, though. That said, we still have the option to release bug fixes, so I think some friction in there is worth compared to maintaining two different implementations plus the code to handle the switch between the two.

I'm planning to pick the new implementation up in 5.2, and in the mean time I think you could hold off from continuing any work on this PR. Sorry we didn't get to review this, but hopefully the change is for the better.

@lb-
Copy link
Member Author

lb- commented Aug 9, 2023

@laymonage thank you, I'm more than happy to align with this direction. Thanks for getting back to me on this.

I'll close this PR and the related isssue and add some notes where needed.

@lb-
Copy link
Member Author

lb- commented Aug 10, 2023

Closing as per comment above, we will not be taking this approach for the date pickers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility javascript Pull requests that update Javascript code
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

🎛️ Migrate datetime picker widgets to a Stimulus Controller w-date
3 participants