Dialog close button comes before dialog contents in tab order

[grncdr]

Describe the bug

When a dialog is opened, the dialog panel is focused (if no element in the dialog has an autofocus attribute).

However, hitting Tab does not focus the first focusable element inside the dialog panel, but instead it focuses the close button.

To Reproduce

Steps to reproduce the behavior:

1. Go to https://codepen.io/grncdr/pen/zYydxoq
2. Click on 'Open Dialog'
3. Hit Tab
4. The Close button (top-right) is focused before the sl-input.

Demo

https://codepen.io/grncdr/pen/zYydxoq?editors=1000

Browser / OS

• OS: Mac
• Browser: latest Chrome, Firefox, and Safari

Additional information

I suspect this might be intentional? If so, consider this a feature request.

I would expect the focus order to be:

1. my content
2. my footer buttons
3. the close button

Such that Shift+Tab would focus the close button first.

If that's too strange/difficult, I'd also be happy to see an option that allows omitting the X from the focus order all together, since Esc does the same thing for users that wish to navigate with the keyboard.

[claviska]

We don't want to explicitly set tab orders in dialogs, so maybe removing the close button from the tab order is the way to go. Any thoughts on this, @KonnorRogers?

[KonnorRogers]

I don't particularly love omitting the close key from the keyboard order...if anything this feels like a bug in our focus trapping.

I'll take a look in the morning and see if I can debug what's happening.

[claviska]

@KonnorRogers we can't use tabindex="-1" because <sl-icon-button> doesn't pass it through to the internal button. We'd have to think about what that API would look like, as it would apply to <sl-button> as well.

[KonnorRogers]

@claviska would inert work? (can't remember if it also disables pointer-events)

[claviska]

Unfortunately no. That will also prevent clicks. 😢

[grncdr]

if anything this feels like a bug in our focus trapping.

Was this avenue explored any further? I tend to agree that omitting the close button entirely from tab order isn't ideal, and I don't actually understand how the current behaviour is happening. I'd assume the DOM tree for the dialog itself is something like this:

<div part="base">
  <div part="header">
    <!-- dialog label slot -->
    <!-- dialog header actions slot -->
    <sl-icon-button name="x"></sl-icon-button><!-- close button -->
  </div>
  <div part="panel"><!-- this element is focused on open -->
    <!-- anonymous slot -->
  </div>
  <div class="dialog-footer">
    <!-- buttons etc. -->
  </div>
</div>

To me it's very mysterious that when the part=panel is focused, hitting tab jumps "backwards" in the DOM instead of to the next focusable element. Have one/both of you already figured out the cause? Is it a ShadowDOM thing or specific to Shoelace/Lit? I might have some time next week to try and create a minimal reproduction without Shoelace in case that could help.

[KonnorRogers]

@grncdr hmmm....this may have to do with our focus trapping code not realizing that a trapped element has already been focused.

I think I may have a fix for this.

[KonnorRogers]

So there's 2 parts to this problem.

1. Our focus trapping utility doesn't currently account for if focus was changed from outside sources. I just fixed this. PRing shortly.
2. The other problem is that the DOM structure you describe isn't the actual DOM structure.

        <div part="panel">
            <header part="header" class="dialog__header">
                <h2 part="title" class="dialog__title" id="title">
                </h2>
                <div part="header-actions" class="dialog__header-actions">
                  <slot name="header-actions"></slot>
                  <sl-icon-button
                    part="close-button"
                   ></sl-icon-button>
                </div>
          </header>
          
          <slot part="body" class="dialog__body" tabindex="-1"></slot>

          <footer part="footer" class="dialog__footer">
            <slot name="footer"></slot>
          </footer>
      </div>

So we initially focus the panel which wraps the header, body, and footer. We could change the initial focus to be the body and fix it that way, I wouldn't necessarily consider it a "breaking change", but it would be a change in behavior perhaps we should make it configurable?

But I managed to solve the first part. You could also throw autofocus onto an element in the body content.

<sl-dialog label="Dialog" class="dialog-overview">
  Lorem ipsum dolor sit amet, consectetur adipiscing elit.
  <sl-input autofocus placeholder="tab to me"></sl-input>
  <sl-button slot="footer" variant="primary">Close</sl-button>
</sl-dialog>

@claviska I guess the only pause would be if you have any other focusable elements in that header area.

[claviska]

Dialogs and drawers both support header actions that should be tabbable.

https://shoelace.style/components/dialog/#header-actions

We could change the initial focus to be the body and fix it that way

I'm not sure that's a good idea. The initial focus moves it to this element, which has the dialog role.

Moving it to the body will be confusing to screen reader users, as it won't be announced as a dialog.

[grncdr]

You could also throw autofocus onto an element in the body content.

Yes, I'd considered that and so far it works for any case I need it to.

Dialogs and drawers both support header actions that should be tabbable.
Moving it [initial focus] to the body will be confusing to screen reader users, as it won't be announced as a dialog.

I am also using header actions in a few cases, and I (very subjectively) would expect them to also come after dialog content in tab order.

---

Idea 1

What about re-ordering the DOM and then visually placing the header actions at the top of the dialog? (e.g. with CSS grid areas)

<div part="panel" role="dialog" ...>
  <header part="header" class="dialog__header">
    <h2 part="title" class="dialog__title" id="title"></h2>
  </header>

  <slot part="body" class="dialog__body" tabindex="-1"></slot>

  <footer part="footer" class="dialog__footer">
    <slot name="footer"></slot>
  </footer>
  
  <div part="header-actions" class="dialog__header-actions">
    <slot name="header-actions"></slot>
    <sl-icon-button
      part="close-button"
     ></sl-icon-button>
  </div>
</div>

Then you get the "expected" (by me, at least... 😅) tab order without having to mess with tabindex or (hopefully) the experience of screen reader users.

Idea 2

The dialog could also implement the same accessibility pattern you'd use for an entire page: inserting an invisible "Skip to content" link at the start of the dialog header.

<div part="panel" role="dialog" ...>
  <header part="header" class="dialog__header">
    <a onClick="..."><slot name="skip-to-content">Skip to content</slot></a>

    <h2 part="title" class="dialog__title" id="title"></h2>
    <div part="header-actions" class="dialog__header-actions">
      <slot name="header-actions"></slot>
      <sl-icon-button
        part="close-button"
       ></sl-icon-button>
    </div>
  </header>

  <slot part="body" class="dialog__body" tabindex="-1"></slot>

  <footer part="footer" class="dialog__footer">
    <slot name="footer"></slot>
  </footer>
</div>

This would be pretty robust, but also maybe still irritating, as it essentially replaces Dialog --Tab--> Close Button --Tab--> First Tabbable Element with Dialog --Tab--> Skip Content Link --Return--> Dialog Body --Tab--> First Tabbable element.

[claviska]

What about re-ordering the DOM and then visually placing the header actions at the top of the dialog? (e.g. with CSS grid areas)

This is a great idea, especially considering the containing panel is already using display: flex. Alas, the close button is in the same container as the title, so moving the entire <header> down will also mess with screen readers. The virtual cursor will go from Panel (role=dialog) => Content => Title => Header Actions (close button, etc.) => Footer.

If we use this approach, we'll need to separate header actions from the header. I'm not sure how feasible that is.

The dialog could also implement the same accessibility pattern you'd use for an entire page: inserting an invisible "Skip to content" link at the start of the dialog header.

In most cases, when the close button is the only header action, this will require the same number of interactions to get to the content. I don't see a real benefit here, except if you quickly tab and hit Enter it wouldn't close. However, that sort of muscle memory probably won't exist unless you use the dialog a lot, in which case users will have learned that doing so closes the dialog.

I decided to look at some other examples to see how they're handling it.

• Bootstrap has the same behavior: https://getbootstrap.com/docs/4.0/components/modal/
• Element Plus has the same behavior (the focus state is subtle though): https://element-plus.org/en-US/component/dialog.html
• Ant Design has the same behavior: https://ant.design/components/modal
• Chakra UI focuses the close button by default (uses :focus-visible so it's not obvious): https://chakra-ui.com/docs/components/modal
• MUI has the same behavior: https://mui.com/joy-ui/react-modal/
• NextUI has the same behavior: https://nextui.org/docs/components/modal

I appreciate why you're asking for this, but it seems like it's coming down to personal preference. I can't find anything in the APG that suggest this is an antipattern.

Note that you can also use the no-header attribute to remove the title and close button and implement them yourself. Here's an example using a regular <button> with tabindex="-1".

https://codepen.io/claviska/pen/abPKBWX

I'm leaning towards this being a wontfix, because so many other libraries behave the same way. While I see your point of view, many other libraries do the same thing and there's no obvious solution unfolding here. If it's bothersome to you, it's probably best to use no-header and either remove the close button completely or add tabindex="-1" to a custom one as shown above.

[grncdr]

I'm leaning towards this being a wontfix, because so many other libraries behave the same way. While I see your point of view, many other libraries do the same thing and there's no obvious solution unfolding here. If it's bothersome to you, it's probably best to use no-header and either remove the close button completely or add tabindex="-1" to a custom one as shown above.

That's totally reasonable. After looking at all the examples you provided, I will probably keep the default behaviour. 😅

However, I want to confirm that #1583 is a separate problem, and I would consider it a bug fix. I really don't mind that the first Tab focuses the close button. The more annoying behaviour is if you focus an input with the mouse, hitting Tab jumps backwards to the close button.

Edit: just saw that you already approved that PR. I think this can be closed.