Focus indicator

Just tested this out and the focus indicator renders well in Chrome and Edge but in Firefox all I see if the blue focus indicator and not the white. I notice your outline change of -1 px, which allows us to see all four borders of the indicator (an improvement) but it doesn't get rendered when FF's blue focus indicator competes with it. I, also, am not a CSS guru so I really can't explain the phenomenon better than that, and even that is a (somewhat) educated guess. During my personal attempts to fix this as shown in the other PR, I used an offset of -2 px to account for the rendering in FF.

It's a tricky one, but if you run this script in the JS console it will put the -2px offset and you'll see the rendering in FF is better. Note the !important isn't necessary for final markup and the # subclasses are just temporary names.

(() => {
  const toggle = document.getElementById('menu-collapse');

  if (!toggle) {
    console.error('Toggle Menu button #menu-collapse was not found.');
    return;
  }

  document.getElementById('toggle-focus-test')?.remove();

  const test = document.createElement('style');
  test.id = 'toggle-focus-test';
  test.textContent = `
    .sidebar-wrapper .sidebar-toggle > button#menu-collapse:focus-visible,
    .sidebar-wrapper .sidebar-toggle > button#menu-collapse.focus-visible {
      outline: 2px solid rgb(238, 238, 238) !important;
      outline-offset: -2px !important;
    }
  `;

  document.head.appendChild(test);
})();

Here are two screenshots, the first showing this PR's current focus indicator in Firefox, and the second with the above JS script applied. The rendering difference is clear:

aria-expanded versus aria-pressed

I don't disagree that expanded may feel like it makes more logical sense since the control's function looks like a disclosure, but since it's distinctly a toggle I'm still of the belief that pressed is the better choice. A toggle's state is on or off, and the icon beside the text implies this switch-like behaviour. Most critically, the disclosures elsewhere in this interface reveal more controls/options to the user. The Toggle Menu button does not reveal any additional function, so while it's visually expanded, it's functionally toggled.

Separately, I think my biggest gripe with the control is that it isn't really distinctly clear what it does when you first enter the interface. What is a toggle menu, exactly? If there were clearer action words like "Show" or "Hide" I think the function would be a lot clearer for everyone. This is just an opinion.

Saying it is pressed has zero meaning

Saying it is pressed has zero meaning

Which I think ultimately means it's an issue with the default state, because I'd argue the default state has no meaning visually either. Default state: disabled toggle icon, user clicks it, the icon represents the enabled state, the menu collapses. Seems a bit counterintuitive—the icon seems to be sending the wrong message. The menu is already "toggled", why is the icon a disabled switch?

All the more reason to change its text to something practical, or change the icon and have aria-pressed="true" on the default expanded state of the menu. "Toggle Menu, pressed" makes a lot more sense in the default state than "Toggle menu, not pressed".

My main point is that it doesn't function like other disclosure widgets in the menu, as in no new controls are revealed and no new functionality is offered, so it's just a state switch from enabled to disabled—a toggle.

Saying it is pressed has zero meaning

Which I think ultimately means it's an issue with the default state, because I'd argue the default state has no meaning visually either. Default state: disabled toggle icon, user clicks it, the icon represents the enabled state, the menu collapses. Seems a bit counterintuitive—the icon seems to be sending the wrong message. The menu is already "toggled", why is the icon a disabled switch?

All the more reason to change its text to something practical, or change the icon and have aria-pressed="true" on the default expanded state of the menu. "Toggle Menu, pressed" makes a lot more sense in the default state than "Toggle menu, not pressed".

My main point is that it doesn't function like other disclosure widgets in the menu, as in no new controls are revealed and no new functionality is offered, so it's just a state switch from enabled to disabled—a toggle. Basically, toggling compact navigation mode on and off.

Saying it is pressed has zero meaning

Which I think ultimately means it's an issue with the default state, because I'd argue the default state has no meaning visually either. Default state: disabled toggle icon, user clicks it, the icon represents the enabled state, the menu collapses. Seems a bit counterintuitive—the icon seems to be sending the wrong message. The menu is already "toggled", why is the icon a disabled switch?

All the more reason to change its text to something practical, or change the icon and have aria-pressed="true" on the default expanded state of the menu. "Toggle Menu, pressed" makes a lot more sense in the default state than "Toggle menu, not pressed".

My main point is that it doesn't function like other disclosure widgets in the menu, as in no new controls are revealed and no new functionality is offered, so it's just a state switch from enabled to disabled—a toggle. Basically, toggling compact navigation mode on and off—just changing the presentation.

Which is exactly what the original report is useless.

Many assistive-technology users will not be able to determine the proper state of the control. As such, they may be blocked from operating the interface or completing the task with confidence.

Other than changing the link to a button and styling it appropriately anything about state is simply wrong. But I am not the expert

Which is exactly why the original report is useless.

Many assistive-technology users will not be able to determine the proper state of the control. As such, they may be blocked from operating the interface or completing the task with confidence.

Other than changing the link to a button and styling it appropriately anything about state is simply wrong. But I am not the expert

-2px on my firefox was no better or different than -1px

So you got the blue indicator with both values?

Yes

I am keeping the focus css as is in this PR even though it is not perfect as it is better solved with #48742 either before or after this is merged

@brianteeman Do you want #48742 to cover the button version of the toggle too?

@Nikhil-sinhaa yes but it cant do that until this is merged OR you can add the focus change for this button to this PR

I have tested this item ✅ successfully on f0e8372

I was able to successfully test this PR fix. Thanks @brianteeman!

I have tested this item ✅ successfully on f0e8372

I was able to successfully test this PR fix. Thanks @brianteeman!

I have tested this item ✅ successfully on f0e8372

I was able to successfully test this PR fix. Thanks @brianteeman!

BEFORE:

AFTER: