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

Dialog review #1504

Merged
merged 14 commits into from Mar 8, 2023
Merged

Dialog review #1504

merged 14 commits into from Mar 8, 2023

Conversation

GomezIvann
Copy link
Collaborator

@GomezIvann GomezIvann commented Mar 1, 2023

Checklist

  • Build process is done without errors and all tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
Summary:

  1. Extra padding-top removed (Dialog Box - padding top #1446). Now, when displayed, the decision of how to avoid the clear action button relies on the user.
  2. The content is now free from development decisions (Dialog || Alert - width overflow control ? #1470). The content is no longer subject to its container display value (removed flex). The decision of how to lay out the components inside the Dialog relies on the user.
  3. Responsive values updated. The width of the dialog when the screen is smaller changes from 800px to 696px.
  4. Fixed problems related to the Escape key. When the dialog didn't have any component to bubble the onkeydown event, the key didn't close the modal box. For example, a Dialog with only text.
  5. New implementation: Focus Lock. A new utility component was added to the DS that keeps the focus on its children and autofocuses the first available element.
  6. Removed several tokens related to the typography of the custom content: fontFamily, fontSize and fontWeight.
  7. Updated specifications with the new values in the tables. Also, removed the previously mentioned tokens from every place on the documentation.

Closes #1446 #1470 #1485 #1496

@GomezIvann GomezIvann marked this pull request as ready for review March 2, 2023 16:54
@Jialecl Jialecl self-requested a review March 3, 2023 11:10
Copy link
Collaborator

@Jialecl Jialecl left a comment

Choose a reason for hiding this comment

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

These are some of the strange behaviours I have found out for now:

If there is an element with a tabIndex > 0 the focus can leave the dialog.
When using the shift tab from the first element it seems to need an extra step to focus on the last element.

lib/src/common/variables.js Outdated Show resolved Hide resolved
@GomezIvann
Copy link
Collaborator Author

GomezIvann commented Mar 3, 2023

These are some of the strange behaviours I have found out for now:

If there is an element with a tabIndex > 0 the focus can leave the dialogue. When using the shift tab from the first element it seems to need an extra step to focus on the last element.

The second case is already fixed. In regards to the first one, I couldn't reproduce it since in my code is working fine, could you please paste yours in a comment?

image

@GomezIvann GomezIvann requested a review from Jialecl March 6, 2023 08:39
@Jialecl
Copy link
Collaborator

Jialecl commented Mar 6, 2023

I just simply added a DxcButton with tabIndex={1} to the dialog.
Related to this issue using the shift tab I was nerver able to focus on the button with positive tabIndex.

@GomezIvann
Copy link
Collaborator Author

GomezIvann commented Mar 6, 2023

I just simply added a DxcButton with tabIndex={1} to the dialog. Related to this issue using the shift tab I was nerver able to focus on the button with positive tabIndex.

I saw the problem. When we change the tabIndex of the inner content to a different value than the Dialog's one, it seems to lose the lock (understandable). I'm currently checking this since I'm not sure how it should work in this case.

@Jialecl
Copy link
Collaborator

Jialecl commented Mar 6, 2023

I just simply added a DxcButton with tabIndex={1} to the dialog. Related to this issue using the shift tab I was nerver able to focus on the button with positive tabIndex.

I saw the problem. When we change the tabIndex of the inner content to a different value than the Dialog's one, it seems to lose the lock (understandable). I'm currently checking this since I'm not sure how it should work in this case.

One more thing I noticed is that if the first element has tabIndex > 0 it is still autofocused even if there are elements with higher priority.

@GomezIvann
Copy link
Collaborator Author

GomezIvann commented Mar 6, 2023

I just simply added a DxcButton with tabIndex={1} to the dialog. Related to this issue using the shift tab I was nerver able to focus on the button with positive tabIndex.

I saw the problem. When we change the tabIndex of the inner content to a different value than the Dialog's one, it seems to lose the lock (understandable). I'm currently checking this since I'm not sure how it should work in this case.

One more thing I noticed is that if the first element has tabIndex > 0 it is still autofocused even if there are elements with higher priority.

That's because that's not how tabIndex works. Here, the main goal is to autofocus the first available focusable item of the Dialog. By changing the value of its tabIndex you are only establishing a different order in the normal sequence of tabbable elements. tabIndex is not used for autofocusing, for that kind of purpose is the autofocus property (which is what we are trying to reproduce).

You can take a look at other implementations to check this:

  1. https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/examples/dialog/
  2. https://www.radix-ui.com/docs/primitives/components/dialog

@Jialecl Jialecl merged commit d4e17f7 into master Mar 8, 2023
@Jialecl Jialecl deleted the gomezivann-dialog-review branch March 8, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants