Skip to content

Conversation

@fscgustavo
Copy link
Contributor

Closes #24150

What I did

  1. Transformed the span trigger elements into button elements.
    1. Button elements already have many implicit accessible behaviors that a span/div does not.
    2. The tag change implied in some CSS changes, so I did it and made some optimizations along the way
  2. Removed the document onkeydown listeners of this context and passed its function to the inputs
    1. The function of the listener was triggered even if the input was not focused.
    2. By passing the function directly to the inputs we avoid necessary executions and restore the native behavior of the HTML button element (every button element outside of the iframe was is not triggered by the enter key)
  3. Improved the acessibility and semantic of the raw button

Before

unable.to.navigate.tab.groups.webm

After

Screen.Recording.2023-10-25.at.19.58.28.mov

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Run a template and search for a story that contains an object control
  2. Add a variety of values and verify if the behavior of the accordion and the other buttons stays the same
  3. Verify if you can trigger the buttons outside the iframe with the enter key
  4. Verify if the enter and that esc key still work inside the object control inputs

This is the object and the path that I used to test:

file path: sandbox/react-vite-default-ts/src/stories/Header.stories.ts
object:

export const LoggedIn: Story = {
  args: {
    user: {
      name: 'Jane Doe',
      exampleObj: {
        key: 'value',
        methodOfObj: function() {
          return true;
        },
      },
      list: ['valueOfList', {foo: 'bar'}],
      method: function() {
        return true
      }
    },
  },
};

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

  1. There are some UI regressions, the left margin for the object/arrays seems to have increased. You can see the differences here in Chromatic, could you take a look at that? https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=653a54b591e4e555d209e0f4

  2. It appears the tabindex order is reversed for "Save"/"Cancel" when adding a new property, could you look at that? See this recording for what I mean:
    Kapture 2023-10-26 at 14 11 19

I'd love you to QA this as well @cdedreuille

@fscgustavo fscgustavo force-pushed the accessible-object-controls branch 3 times, most recently from 588fb59 to 67972e2 Compare October 29, 2023 23:22
@fscgustavo
Copy link
Contributor Author

There are some UI regressions, the left margin for the object/arrays seems to have increased. You can see the differences here in Chromatic, could you take a look at that?

The margin-right of the button was adjusted ✔️

Do you think the Safari layout variation is still bad for the control?

@fscgustavo fscgustavo requested a review from JReinhold October 29, 2023 23:48
Copy link
Contributor Author

@fscgustavo fscgustavo left a comment

Choose a reason for hiding this comment

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

I forgot to submit the comment 😅

<span className="rejt-edit-form" style={style.editForm}>
{inputElementLayout} {cancelButtonElementLayout}
{editButtonElementLayout}
{inputElementLayout} {editButtonElementLayout}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JReinhold

I removed the order style of the button and adjusted the order of the elements in the DOM

Thanks to your comment, I was able to see that the edit buttons never appear due to a display none style

Are there any intentions to make these buttons appear again, or can we remove this piece of code?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any intentions to make these buttons appear again, or can we remove this piece of code?

I think we can remove it just fine.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Sorry for the delay and thanks for your patience. I think this looks great.

If you could resolve the merge conflicts then I think this is good to go.

@fscgustavo fscgustavo force-pushed the accessible-object-controls branch 2 times, most recently from 880edfb to 5916e29 Compare May 15, 2024 19:08
@nx-cloud
Copy link

nx-cloud bot commented May 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0282dbb. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@fscgustavo fscgustavo force-pushed the accessible-object-controls branch from 5916e29 to f422c90 Compare May 15, 2024 19:33
@JReinhold JReinhold changed the title Improve the accessibility of the object control buttons Controls: Improve the accessibility of the object control May 15, 2024
@JReinhold
Copy link
Contributor

There's a visual regression introduced for the arrays, you can see the visual diff here: https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=66450f76b7ff051578a13b9b

And in this story: https://635781f3500dd2c49e189caf-lcdshmxzdy.chromatic.com/?path=/story/blocks-controls-object--array

It looks like an li bullet is visible now, which it probably shouldn't be.

@fscgustavo fscgustavo force-pushed the accessible-object-controls branch from f422c90 to 0282dbb Compare May 15, 2024 20:14
@fscgustavo
Copy link
Contributor Author

There's a visual regression introduced for the arrays, you can see the visual diff here: https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=66450f76b7ff051578a13b9b

And in this story: https://635781f3500dd2c49e189caf-lcdshmxzdy.chromatic.com/?path=/story/blocks-controls-object--array

It looks like an li bullet is visible now, which it probably shouldn't be.

@JReinhold The bug has been resolved! It was likely lost during the conflict resolution.

I have also deleted the edit button that was already hidden with CSS.

Thanks for getting back to me! I am available if any new important changes are required :)

@Sidnioulz
Copy link
Member

@JReinhold I'd suggest merging this PR's content as it is. I've rebased and opened #31581 so we get a clean CI status and can work on merging this.

I'll also update the issue with additional steps to close it, as I don't believe this PR itself is enough to make the component accessible. I just don't feel comfortable giving @fscgustavo more work a year after he has submitted this, and I think it would be better to continue work on a follow-up PR.

@Sidnioulz Sidnioulz closed this May 27, 2025
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.

[A11y]: Object control is not accessible (kb nav, aria markup)

4 participants