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

Site Editor: Tab order of editor canvas and resize handle #52602

Open
mirka opened this issue Jul 13, 2023 · 5 comments
Open

Site Editor: Tab order of editor canvas and resize handle #52602

mirka opened this issue Jul 13, 2023 · 5 comments
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@mirka
Copy link
Member

mirka commented Jul 13, 2023

In #52443, it was raised that the tab order of the editor canvas and resize handle was unintuitive:

One thing I found a bit confusing is that the 'Drag to resize' handle is placed in the tab sequence after the iframe. I would expect to find it between the navigation and the iframe so that it also matches the visual order.

It would be nice if we could flip the tab order. However, it isn't very easy because the element order depends on the re-resizable library under the hood.

Also, in the general case, resize handles can be on any edge or corner. It's only in this specific case where the handle is only on the left edge. So I do see an argument to optimize for consistency in the general case (for example the one in Style Book with two handles), and have all handles come after the frame.

@mirka mirka added [Type] Enhancement A suggestion for improvement. [a11y] Keyboard & Focus [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Jul 13, 2023
@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Jul 18, 2023
@afercia
Copy link
Contributor

afercia commented Jul 18, 2023

To my understanding, the problem with the ResizableBox component and the re-resizable library is that it is meant for a different use case :)

By default, re-resizable draws a rectangular box with 8 handles: 4 for the corners and 4 for the edges. Screenshot:

Screenshot 2023-07-18 at 14 11 15

To do so, re-resizable draws an overlay above the element that is resizable and in the DOM order the overlay comes after.

It is possible to hide or disable the handles and keep just one or two of them but regardless they will always come after in the DOM order. I'm not sure there is a way to hack around the re-resizable library but anyways I'm not sure hacking it would be future-proof.

I tend to think the ResizableBox component based on re-resizable is just not the right tool for our use case.

Worth reminding that the DOM order problem occurs also in the Style Book where there is an additional problem:

  • Both handles come after the resizable content.
  • The right handle is placed in the DOM before the left one so the Tab order is from right to left, which is a totally unexpected user experience.

Marking this issue as a bug because the mismatch between visual order and DOM order is a WCAG violation.

@afercia
Copy link
Contributor

afercia commented Jan 2, 2024

Looking into this after some time. I'd think the best way to solve this issue would be:

  • either submit a change request upstream
  • or fork re-resizable and build our own, so that we can change its render method.

Looking at re-resizable code:

https://github.com/bokuweb/re-resizable/blob/0f6b2ddcbab2a2864c209e0a59a67b6d51adab86/src/index.tsx#L949-L955

    return (
      <Wrapper ref={this.ref} style={style} className={this.props.className} {...extendsProps}>
        {this.state.isResizing && <div style={this.state.backgroundStyle} />}
        {this.props.children}
        {this.renderResizer()}
      </Wrapper>
    );
  • renderResizer renders the resizer overlay, which contains the resize handles.
  • The resizer is always rendered after the resizable element (the children passd via props).
  • Instead, what we need is;
    • An option to render the resizer before.
    • Even better: a way to render each resize handle before or after the resizable element.

@afercia
Copy link
Contributor

afercia commented Sep 27, 2024

After #65637 it would be good to look again into this issue.

@t-hamano
Copy link
Contributor

t-hamano commented Nov 6, 2024

It seems that bokuweb/re-resizable#827, which was submitted to the re-resizable library to solve this problem, caused a regression 😅 Therefore, the PR was reverted.

I can think of three approaches:

  • Resubmit a different PR to the re-resizable library that does not cause a regression.
  • Create a new component that does not depend on external libraries, such as ResizableBoxV2.
  • Wait for the new reading-flow css to be implemented in browsers. This is not yet implemented in major browsers, but it may be the simplest approach.

@stokesman
Copy link
Contributor

stokesman commented Nov 6, 2024

  • Create a new component that does not depend on external libraries, such as ResizableBoxV2.

I’ll add that re-resizable is ~50kB. Ideally we could manage to do this with a backwards compatible API and not have to export a new component while simultaneously reducing the final bundled size because we could remove that dependency. For the dragging behavior, we could utilize @use-gesture/react which is already bundled in the components package. From there it’s mostly a matter of implementing the UI around that. #66735 happens to serve as an example of how (at least in that use case) the current API/implementation of ResizableBox doesn’t seem to simplify the implementation of a keyboard focusable resizable pane.

  • Wait for the new reading-flow css to be implemented in browsers. This is not yet implemented in major browsers, but it may be the simplest approach.

Thanks for sharing the link, I hadn’t seen that and it would be handy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

5 participants