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

fix(TimePicker): re-add ability to append to document body #7043

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Mar 10, 2022

What: Closes #6942

I also updated how the TimePicker gets appended to its parent by passing in a parent value to the menuAppendTo prop. This should work more similarly to the Dropdown component, and should both resolve the issue of scrollbars appearing in Modals and also allow access to the TimePicker menu via Voice Over. This can be tested by going to the the below page on the build workspace and adding the following snippets of code as instructed.

Link: PatternFly Modal

On line 2, import TimePicker:

import { Modal, Button, TimePicker } from '@patternfly/react-core';

To test appending to a parent, before line 43 (this should come before the closing </Modal> tag) add:

<div>
  <TimePicker menuAppendTo="parent" />
</div>

To test appending to document body, before line 43 add:

<div>
  <TimePicker menuAppendTo={() => document.body} />
</div>

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Mar 10, 2022

@thatblindgeye
Copy link
Contributor Author

@marusak Would you be able to test things in the preview build above? I included some instructions in my initial comment to test the changes to the TimePicker both for appending to document.body and for appending to a parent element. I would be most interested in whether appending to that parent element works as you might expect within a modal, as there's been known accessibility issues when appending dropdowns and similar menus to document.body, and I'm hoping this can be a step in the right direction for resolving two issues at once.

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

So appending it to a parent seemed to work fairly well, and I only had two comments about that interaction:

  • The menu's label isn't getting announced for some reason (it just says "menu")
  • There seems to be no way to escape the menu easily without closing the entire modal. If you press the Esc key, it closes the modal (also true with keyboard)

With appending to document body, I had a lot more issues. The label was announced, interesting enough, but I couldn't figure out any way to get into the menu with VO. It makes me wonder if it's getting disabled like the rest of the document behind the modal?

@thatblindgeye
Copy link
Contributor Author

@jessiehuff based on prior convo, I added a warning to the examples regarding appending to document.body. I've also made the following changes, and would like to hear your thoughts on them:

  • removed the functionality of the menu automatically opening when the input receives focus (similar to the Select typeahead variant, as well as the DatePicker not automatically opening a calendar on focus)
  • in response to the above, added an aria-haspopup attribute to the input to notify users that a menu is attached to the input; keyboard users who do not user a screen reader would still be informed there is a menu once they start typing
  • changed the menus aria-labelledby attribute to an aria-label attribute that uses the passed in aria-label prop; using the former attribute would cause the menu to be labelled as the input's current value, e.g. "3:35 AM" or "hh mm", whereas I think using the aria-label could provide better context.

There is still an issue of the menu being announced with a generic "menu" label by VO when the menu opens. Fixing that might be better for a separate issue, as well as possibly changing the behavior when the menu opens (i.e. should a menu item receive focus when the menu is opened via arrows keys).

@jessiehuff
Copy link
Contributor

So when testing it out, I couldn't seem to use it by keyboard or by VO. :( I think your points above make sense though.

@thatblindgeye
Copy link
Contributor Author

That's odd; I was able to get into the TimePicker via keyboard and VO. @nicolethoen would you be able to test out as well and see what behavior you get? If you and Jessie have the same behavior then it may require seeing the interaction in action.

@nicolethoen
Copy link
Contributor

I see it working in both keyboard and VO as well.
@kmcfaul do you think you could test it out?

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

Hmm maybe my VO was acting up earlier. Appending to parent works, but as usual, not the document body. I think that's expected based on what we know right now though, right? And the other PR adds the verbiage to the TimePicker about that so I suppose this is good then. 😄

@tlabaj tlabaj merged commit d20541b into patternfly:main Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimePicker: menuAppendTo body not working
5 participants