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

feat(listbox-popover): Tab stop for search #1097

Merged
merged 23 commits into from
Mar 7, 2023

Conversation

a-m-dev
Copy link
Member

@a-m-dev a-m-dev commented Mar 2, 2023

Motivation

we need to have a tab stop in search field in sn-table side according to our a11y standards. Since we can't use the useKeyboard () hook directly inside of nebula, we need to mimic it's return values and in this case passing { enabled: false } into ListBoxSearch as keyboard prop does the job for us!

Screen.Recording.2023-03-02.at.09.54.26.mov
Screen.Recording.2023-03-02.at.09.57.26.mov

Update:

now the logic updated and:

  1. by default, when we open listbox popover -> focus is on search field
  2. unless user sends in autoFocus: false as option from where embeds the popover
    (NOTE: 👆that is only for popover mode!)

Requirements checklist

  • Api specification
    • Ran yarn spec
      • No changes OR API changes has been formally approved
  • Unit/Component test coverage
  • Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

  • Add code reviewers, for example @qlik-oss/nebula-core

@LiKang6688
Copy link
Contributor

When the listbox is shown, the focus should be on the search box, it will be in other PRs?

@a-m-dev
Copy link
Member Author

a-m-dev commented Mar 2, 2023

I don't think we've discussed that. We only needed to have a tab stop for search field. Is that some new requirement that I'm missing maybe?

@haxxmaxx
Copy link
Collaborator

haxxmaxx commented Mar 2, 2023

I think that would be nice, since you press "search" to open it. It is also the way it works with the old table

@a-m-dev
Copy link
Member Author

a-m-dev commented Mar 2, 2023

then i think that should come as an option to the popover, because we can have popover on a button which we may not want to have it focused by default (mash up example), will update the pr soon

@haxxmaxx
Copy link
Collaborator

haxxmaxx commented Mar 2, 2023

then i think that should come as an option to the popover, because we can have popover on a button which we may not want to have it focused by default (mash up example), will update the pr soon

@Caele thoughts on this?

@niekvanstaveren
Copy link
Collaborator

then i think that should come as an option to the popover, because we can have popover on a button which we may not want to have it focused by default (mash up example), will update the pr soon

@Caele thoughts on this?

I would say that this is actually the correct behaviour from accessibility perspective. The real issue is probably the design of the popover. We actually faced the same issue with the old table before. If you focus the search field directly how would you know that you have skipped some buttons?

Copy link
Collaborator

@Caele Caele left a comment

Choose a reason for hiding this comment

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

This should work nicely I think. But what Niek mentioned might need some extra stuff, to make sure the user understands that focus shifted.

@a-m-dev
Copy link
Member Author

a-m-dev commented Mar 3, 2023

Yes, I think that requires some design discussions and thinking deeply about what we might take action on it (another PR after we have concrete decisions on it)

Copy link
Contributor

@LiKang6688 LiKang6688 left a comment

Choose a reason for hiding this comment

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

LGTM. 👍 Would be nice to update the description as well.

Copy link
Collaborator

@haxxmaxx haxxmaxx left a comment

Choose a reason for hiding this comment

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

As Niek says, the problem is already there in the old listbox. Since it is not really a downgrade, but rather aligning the new and the old, I don't think we have to consider it in this PR.

@a-m-dev a-m-dev force-pushed the htm/listbox-popover-tabstop-for-search branch from 46a6b24 to 99ecd50 Compare March 6, 2023 14:11
@@ -35,6 +35,7 @@ export default function ListBoxPopover({
app,
fieldName,
stateName = '$',
autoFocus,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
autoFocus,
autoFocus = true,

and can remove the ?? below

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can, but saw this comment after merge, sorry 😅

@Caele Caele merged commit a65e7a1 into qlik-oss:master Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants