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

Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month #2618

Merged
merged 11 commits into from
Apr 4, 2023

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Feb 14, 2023

Provides partial resolution of #2581.
The remainder of the resolution is provided by #2643.

This PR changes behavior of commands for next and previous month and year In the combobox date picker when the currently focused date does not exist in the previous or next month or year. With these changes, if the same day number does not exist in the newly displayed month, the last day of the newly displayed month receives focus.

Preview of revised combobox date picker example in Netlify.


WAI Preview Link (Last built on Tue, 04 Apr 2023 01:43:50 GMT).

@jongund jongund requested review from mcking65 and jnurthen February 14, 2023 16:58
@mcking65 mcking65 changed the title Fixed bug in previous and next month and year Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month Feb 14, 2023
@mcking65
Copy link
Contributor

@jongund

Would you like to update the keyboard documentation table, or would you like me to do that?

I am surprised we are not testing this behavior. I don't see any changes to test/tests/combobox_datepicker.js, yet all tests pass. Are we missing some tests?

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Pull Request Review.

The full IRC log of that discussion <Jem> Topic:Pull Request Review
<Jem> https://github.com//pull/2618
<jugglinmike> github: https://github.com//pull/2618
<jugglinmike> jongund: I'll update the language as Matt_King has requested
<jugglinmike> Matt_King: Are there missing regression tests here?
<jugglinmike> jongund: We didn't have any tests for the condition where the previous or next date was not the current date. Should we add tests for that?
<jugglinmike> Matt_King: Are there any "page up"/"page down" tests at all?
<jugglinmike> jongund: There are a ton of unimplemented tests for certain keystrokes [lists specific keyboard keys]
<jugglinmike> Matt_King: We don't necessarily have to fix that gap with this pull request (though it is a great opportunity to do so)
<jugglinmike> jongund: I'm going to try to integrate some other test files in, along with their documentation

@charmarkk charmarkk requested a review from shirsha February 28, 2023 19:24
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month by jongund.

The full IRC log of that discussion <jugglinmike> Subtopic: Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month by jongund
<jugglinmike> github: https://github.com//pull/2618
<jugglinmike> jongund: All the test cases finished; I'll check them in later today
<jugglinmike> jongund: I've updated the documentation as well
<jugglinmike> Matt_King: We need a review checklist
<jugglinmike> Jem: I'll add that
<jugglinmike> Matt_King: We only need to verify the functionality and the editorical review, but there's no changes to CSS. Only two files changed, just the JavaScript and HTML files.
<jugglinmike> Matt_King: We should have at least one reviewer besides myself
<jugglinmike> Siri: I will perform the functional review

@a11ydoer a11ydoer requested review from alflennik and removed request for jnurthen March 7, 2023 19:41
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month by jongund · Pull Request #2618 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> Subtopic: Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month by jongund · Pull Request #2618 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2618
<jugglinmike> Matt_King: Do we have reviewers assigned?
<jugglinmike> Jem: Yup: shirsha, Matt_King, and James
<jugglinmike> Matt_King: I don't remember James agreeing to that, so we should remove them
<jugglinmike> Jem: done
<jugglinmike> Matt_King: shirsha will be reviewing for functionality, and I think I asked AlexFlennikan to perform code review
<jugglinmike> jongund: It also updates tests
<jugglinmike> jongund: there's a file that was changed that shouldn't have been changed, so I'll fix that
<jugglinmike> Matt_King: I'll do editorial and functional review

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

I reviewed the functionality of the widget and I was happy to find that it worked super reliably with both a mouse and keyboard, even with lots of code changes.

One thing I did was to build the APG website and try the widget in that context. While everything works, I did notice that the table has some styling which makes it less aesthetically pleasing than the non-website version. I do think this is a preexisting issue, but in any case below are some changes that would make the styling consistent in both contexts:

  .combobox-datepicker .dates {
      width: 320px;
      padding-left: 1em;
      padding-right: 1em;
      padding-top: 1em;
+     border-collapse: separate; /* Needed for website */
  }
  .combobox-datepicker .dates th, #myDatepicker .dates th {
      padding: 0;
+     /* Needed for website */
+     color: black;
+     background: transparent;
+     border: none;
  }

One thing I noticed is that hovering the mouse over the active date results in poor contrast, so I would suggest a change like this:

  .combobox-datepicker .dates td:hover {
    background-color: rgb(218, 231, 251);
+   color: black;
  }

I ran the tests and they passed. One minor issue is the fact that there is a lot of logging output coming from line 323 of the test file. I think that's a preexisting issue but it might be nice to take this opportunity to remove the noise from the test output.

@jongund
Copy link
Contributor Author

jongund commented Mar 12, 2023

@alflennik
Thank you for the testing I will update the CSS based on your comments.

@mcking65 mcking65 requested a review from alflennik March 21, 2023 18:48
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month.

The full IRC log of that discussion <jugglinmike> Subtopic: Combobox Date Picker Example: Change previous and next month and year behavior for dates near end of month
<jugglinmike> github: https://github.com//pull/2618
<jugglinmike> Matt_King: AlexFlenniken left some review comments
<jugglinmike> Matt_King: Did you address those, jongund?
<jugglinmike> jongund: Yup. I adjusted the stylesheet to override the default styles
<jugglinmike> Matt_King: I'm re-requesting review from you, AlexFlenniken
<jugglinmike> Matt_King: It's also awaiting review from Siri and me
<jugglinmike> Matt_King: We should probably ping Siri

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Jon!

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

@jongund All good!

I made some minor changes to the wording of the page up and down documentation that we'll want to carry forward into the date picker dialog documentation as well.

@mcking65 mcking65 removed the request for review from shirsha April 4, 2023 03:21
@mcking65 mcking65 merged commit ce0336b into main Apr 4, 2023
@mcking65 mcking65 deleted the bugfix/issue-2268 branch April 4, 2023 03:24
@mcking65 mcking65 added this to the H1/2023 Content Updates milestone Apr 4, 2023
mcking65 added a commit that referenced this pull request Jun 6, 2023
…nce with APG code style guide (pull #2643)

This PR complements PR #2618 to provide the remaining changes to resolve issue #2581.

This PR changes behavior of commands for next and previous month and year In the date picker dialog when the currently focused date does not exist in the previous or next month or year. With these changes, if the same day number does not exist in the newly displayed month, the last day of the newly displayed month receives focus.

In addition to the above enhancement, this PR also includes the following changes:
* Updates coding practices to use `class` constructor and pointer events
* Updates documentation of page up and page down commands
* Updates regression tests for page up and page down commands

---------

Co-authored-by: Matt King <a11yThinker@gmail.com>
@mcking65 mcking65 added bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern labels Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants