Make WordPress Core

Opened 7 months ago

Closed 23 hours ago

#60808 closed defect (bug) (worksforme)

Twenty Twenty-Four: Black outline around the Navigation block submenu-expanding button

Reported by: jordesign's profile jordesign Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.5
Component: Bundled Theme Keywords: 2nd-opinion needs-testing has-patch changes-requested
Focuses: accessibility Cc:

Description

This appears to be a regression introduced in https://core.trac.wordpress.org/changeset/57739

Brought over from the Gutenberg repo: https://github.com/WordPress/gutenberg/issues/59944


Description

When a sub-menu item is present, when clicking on the sub-menu item or on the parent menu item, a black border is shown around the items.

https://cldup.com/kNOVprCJ_d.png

Video: https://cloudup.com/cm8LVp_p6kN

Step-by-step reproduction instructions

  1. go to the site editor
  2. add a navigation block
  3. add an item and a sub item
  4. publish changes and visit the site
  5. click on the sub-menu item and on the parent item
  6. see the black border

Environment info

WordPress 6.5 RC2
2024 theme

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

Attachments (4)

60808.diff (551 bytes) - added by sabernhardt 7 months ago.
switches :focus to :focus-visible
details-block-focus.png (3.6 KB) - added by wildworks 7 months ago.
Custom focus outline causes an unnatural outline to be displayed when clicking the summary element in the Details block
60808-2.diff (590 bytes) - added by wildworks 6 months ago.
Add CSS for focus control to navigation submenu block
additional-css.png (8.1 KB) - added by wildworks 5 months ago.
The styles defined in theme.json are displayed in the global style additional CSS panel

Download all attachments as: .zip

Change History (57)

#1 @poena
7 months ago

  • Focuses accessibility added
  • Keywords 2nd-opinion added

Hi
This is not a bug. While it is a change, it is an intentional accessibility improvement.

Last edited 7 months ago by poena (previous) (diff)

#2 @get_dave
7 months ago

Noting that this applies to all links not just Navigation submenus.

@poena If you have a moment, are you able to advise on where it was added so I can take a look at the rationale behind it?

I've not seen this approach taken before and would be open to learning more to improve my own knowledge.

#3 @poena
7 months ago

Yes, I was on my mobile and it was already linked from the Gutenberg issue, so I did not make the time add it.

The original issue was #60334.

In version 2.2 of WCAG, there is a new AAA level requirement that says that the focus style should be at least 2px thick.

https://www.w3.org/WAI/WCAG22/Understanding/focus-appearance.html
https://www.w3.org/TR/WCAG22/#focus-appearance

WordPress does not officially require WCAG 2.2, only version 2.1, and at level AA, not level AAA. There are discussions and arguments for following this requirement as a best practice in the ticket.

#4 @tomxygen
7 months ago

Although it may be intentional, it is very ugly.
Any chance you could suggest me some CSS to get rid of it?

@sabernhardt
7 months ago

switches :focus to :focus-visible

#5 @sabernhardt
7 months ago

  • Component changed from Themes to Bundled Theme
  • Keywords has-patch added
  • Summary changed from Twenty Twenty Four: Black border around the navigation items when sub items are present to Twenty Twenty-Four: Black outline around the Navigation block submenu-expanding button
  • Version changed from 6.4.3 to trunk

If the submenus are set to open on click, the focus outline is not ideal when someone uses a mouse on the toggle button.

@wildworks suggested :focus-visible (ticket:60535#comment:19), which I should have given more consideration. It applies the 2px solid style when navigating by keyboard, matching the focused element's text color unless the site defines it differently. However, :focus-visible does not add the outline in situations such as clicking the submenu toggle button or opening the context menu ("right-click").

#6 @sabernhardt
7 months ago

  • Keywords needs-testing added

#7 @sabernhardt
7 months ago

  • Milestone changed from Awaiting Review to 6.5.1

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


7 months ago

#9 @wildworks
7 months ago

60808.diff appears to work as expected for me.

However, :focus-visible does not add the outline in situations such as clicking the submenu toggle button or opening the context menu ("right-click").

WCAG describes it as "When the keyboard focus indicator is visible, an area of the focus indicator meets all the following". In other words, it seems like a proper focus outline should only be visible when using the keyboard, and not a requirement when using the mouse.

However, I think it's basically a good thing that the focus is displayed even when using the mouse, so if anyone knows of a good approach, I'd like to hear your ideas.

Another thing I noticed about this issue is that when I click on the summary element in the Details block, an unnatural outline appears. This is due to the fact that overflow: hidden; is applied to the block. However, this may be a problem that should be fixed separately on the Details block side.

@wildworks
7 months ago

Custom focus outline causes an unnatural outline to be displayed when clicking the summary element in the Details block

#10 @sabernhardt
7 months ago

Yes, the missing/incomplete Details block focus outline is a separate issue and could have happened in any theme since 6.3. The bottom line shows when the element is open, but the summary focus does not appear at all when closed. Do you want to open a new Gutenberg issue for that?

#11 @wildworks
7 months ago

Do you want to open a new Gutenberg issue for that?

Yes, I reported it here: https://github.com/WordPress/gutenberg/issues/60269

#12 @poena
6 months ago

I believe comment:9 is correct. would like to ask Amber @alh0319 for their opinion, as the original reporter.

#13 @sabernhardt
6 months ago

#60923 was marked as a duplicate.

#14 @sabernhardt
6 months ago

#60923 was marked as a duplicate.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#16 @davidbaumwald
6 months ago

  • Milestone changed from 6.5.1 to 6.5.2

Milestone renamed

#17 @jorbin
6 months ago

  • Milestone changed from 6.5.2 to 6.5.3

#18 @alh0319
6 months ago

I don't think we should change to :focus-visible from :focus. I think @wildworks is right when saying that:

However, I think it's basically a good thing that the focus is displayed even when using the mouse

There may be cases where people are mouse users but have limited mobility or on a mobile phone, where having the focus indicator helps them to know that they are clicking on the correct element, particularly if tap targets are close together. (Parkinsons, for example.)

Let me alert the broader accessibility team to this ticket and see if we can get several opinions.

This ticket was mentioned in Slack in #accessibility by amberhinds. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#21 @jorbin
6 months ago

  • Keywords needs-patch added; has-patch removed

Based on the idea that switching to :focus-visible wouldn't be beneficial, I think we will need a new patch

@wildworks
6 months ago

Add CSS for focus control to navigation submenu block

#22 @wildworks
6 months ago

I submitted a 60808-2.diff trying a more limited solution. This accomplishes the following for the Navigation Submenu block with "Open on click" enabled:

  • When clicking or right-clicking a menu that has a submenu via the mouse, the outline will be displayed as before.
  • When a submenu is opened, the menu has aria-expanded="true" applied. This will apply the newly added CSS from the patch and the menu's focus outline will disappear.
  • If the focus is moved using only the tab key on the keyboard, the focus outline of the menu will not disappear even if a submenu is opened.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#24 @sabernhardt
6 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 6.5.3 to 6.6

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#26 @jorbin
6 months ago

  • Milestone changed from 6.6 to 6.5.3

Moving back into the 6.5.3 milestone since this would be a nice thing to fix.

@sabernhardt if you think we should punt, can you please share some rationale?

#27 @wildworks
5 months ago

I started thinking that solving this problem via theme.json might not be ideal. Because styles defined in the css field can be overridden by the user, but I don't think this style should be overridden.

Unfortunately, there is currently no way to enqueue styles only to the frontend via theme.json.

What do you think about using the wp_enqueue_scripts() hook instead?

@wildworks
5 months ago

The styles defined in theme.json are displayed in the global style additional CSS panel

#28 @poena
5 months ago

I disagree with adding and enqueueing a stylesheet because it's a convention that block themes are trying to move away from; it is part of reducing the complexity of theme building.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


5 months ago

#30 @jorbin
5 months ago

  • Milestone changed from 6.5.3 to 6.6

With time running out to work on 6.5.3 and a lack of an agreed-upon direction, I'm moving this to 6.6. If a solution presents itself, I think it would be worth considering including in either a different minor or in an out-of-cycle theme-only release.

This ticket was mentioned in Slack in #accessibility by joesimpsonjr. View the logs.


5 months ago

#32 @karmatosed
5 months ago

I support what @poena is saying here. What are the other options based on that?

This ticket was mentioned in Slack in #core-themes by karmatosed. View the logs.


5 months ago

#34 @hmbashar
4 months ago

  • Keywords needs-testing-info added

#35 @hmbashar
4 months ago

Test Report

Description

I don't see the around border after clicking or hovering, check the screenshot https://prnt.sc/rTkS4_68rv1i

Environment

  • WordPress: 6.6-alpha-57778-src
  • PHP: 8.3.7
  • Server: nginx/1.25.4
  • Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.3.7)
  • Browser: Chrome 125.0.0.0
  • OS: macOS
  • Theme: Twenty Fifteen 3.7
  • MU Plugins: None activated
  • Plugins:
    • FakerPress 0.6.6
    • Test Reports 1.1.0

#36 @wildworks
4 months ago

I disagree with adding and enqueueing a stylesheet because it's a convention that block themes are trying to move away from; it is part of reducing the complexity of theme building.

I understand this convention. However, if you go ahead with the approach of adding CSS to theme.json, we should be aware that it will only work for users who have not updated their Additional CSS.

If the user has already updated additional CSS for the block to which the patch is trying to apply CSS, the CSS defined in theme.json will not be applied.

Then, as I already mentioned, additional CSS can be overridden by the user.

This is my concern and why I suggested an alternative approach of enqueuing styles via PHP.

This ticket was mentioned in Slack in #accessibility by rcreators. View the logs.


4 months ago

#38 @sabernhardt
4 months ago

  • Milestone changed from 6.6 to 6.7

#39 @poena
4 months ago

Yes, that the updated theme.json is not used if the user has already added custom CSS is a problem that I did not consider. That is a valid argument.

I don't think that the problem, which is that some people find the styles ugly (as opposed to something being broken) validates the proposed solution.

One could wish that the editor implemented better ways to control all link states, but there is no focused work on that area right now.

#40 @faiz786
4 months ago

Test Report

I have tested the sub drop-down menu by clicking on it. So now parent menu not displayed the black order around on it.

https://conceptopensource.com/wp-demo/test-drop-down.gif

This ticket was mentioned in Slack in #core-themes by karmatosed. View the logs.


3 months ago

#42 @karmatosed
3 months ago

  • Keywords needs-testing-info removed

As of today the situation with this appears to be:

  • There is a suggested solution.
  • This solution doesn't just include theme.json but also a stylesheet.
  • The recommended method is to just use theme.json.

As this has been released a little while now with this issue I think we need to either:

  • Agree to keep as is.
  • Have a solution that just impacts theme.json and keeps to the block theme approach.

As @poena rightly noted there is work going on with links currently will give options very soon in core itself outside of themes.

My recommendation would be to close this in favour of a core solution if the only option is a stylesheet.

#43 @poena
3 months ago

I have been thinking a lot about this, I am still torn, but I have changed my mind.

  • Even if the editor comes up with controls for styling states, it will only support those blocks, and it will not work for all focusable elements.
  • Changing the default style for focusable elements in core/Gutenberg is not an option unless there is a project wide decision to require WCSAG level AAA. So the alternative is still to solve it theme by theme.
  • Users who has already saved additional CSS before updating the theme does not benefit from the CSS in theme.json.

I think the CSS that was previously added should be moved from theme.json to style.css, updated with the change proposed in this ticket, and enqueued.
And that a new file for the proposed navigation block CSS should be created and enqueued conditionally. I mean enqueued the same way as the button stylesheet.

Last edited 3 months ago by poena (previous) (diff)

#44 @wildworks
3 months ago

I think the CSS that was previously added should be moved from theme.json to style.css, updated with the change proposed in this ticket, and enqueued.
And that a new file for the proposed navigation block CSS should be created and enqueued conditionally. I mean enqueued the same way as the button stylesheet.

If we consider this ticket a bug, I agree with this approach. I know that there is a convention to avoid using PHP and stylesheets in block themes, but if there is a solution, I think it is better to try it.

This ticket was mentioned in Slack in #core-themes by kafleg. View the logs.


2 months ago

#46 @kafleg
2 months ago

  • Keywords changes-requested added
  • Resolution set to invalid
  • Status changed from new to closed

#47 @kafleg
2 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


8 weeks ago

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


8 days ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


23 hours ago

#52 @joedolson
23 hours ago

  • Milestone changed from 6.7 to Future Release

With 6.7 just around the corner and no movement on this, I'm moving it to future release.

#53 @joedolson
23 hours ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from reopened to closed

Looking more closely, what I'd say is that we should close this issue. The issue raised is a focus outline, which is expected and normal, and not something we want to fix. The comments in the ticket are discussing methods of adding additional CSS for block themes, and that's really a separate issue.

I suggest raising the larger topic as a new ticket; I'm going to close this issue.

Note: See TracTickets for help on using tickets.