Make WordPress Core

Opened 13 months ago

Last modified 2 weeks ago

#37013 assigned defect (bug)

Minor Fixes: "Toggle indicator" in pages have focus color while the same in widgets/menus have none.

Reported by: monikarao Owned by: afercia
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-screenshots needs-testing
Focuses: ui, accessibility Cc:


Toggle indicator in pages/post has visible focus color(blue border radius) on indicator but this is not present on Widget and Menus. It should display same focus effect on "Toggle Indicator" in Widgets and Menus.

Testing Environment:-
OS: Windows 10
Browser: Chrome 50.0.2661.102 , IE-11 , Firefox 46.0.1
WP : 4.5.2
Server-Environment : VVV

Attachments (12)

toggle-indicator-pages.png (14.1 KB) - added by monikarao 13 months ago.
Toggle Indicator on Pages
toggle-indicator-menu.png (16.9 KB) - added by monikarao 13 months ago.
Toggle Indicator on Menu
toggle-indicator-widget.png (19.4 KB) - added by monikarao 13 months ago.
Toggle Indicator on Widget
37013.diff (2.4 KB) - added by xavortm 12 months ago.
37013.2.diff (3.9 KB) - added by mihai2u 12 months ago.
37013.3.diff (6.7 KB) - added by mihai2u 12 months ago.
37013.4.diff (7.6 KB) - added by mihai2u 2 months ago.
menu-togglers.jpg (9.2 KB) - added by mihai2u 2 months ago.
widgets-togglers.jpg (11.1 KB) - added by mihai2u 2 months ago.
37013.5.diff (7.6 KB) - added by mihai2u 2 months ago.
37013.6.diff (379.2 KB) - added by mihai2u 7 weeks ago.
37013.7.diff (7.7 KB) - added by mihai2u 7 weeks ago.

Download all attachments as: .zip

Change History (38)

13 months ago

Toggle Indicator on Pages

13 months ago

Toggle Indicator on Menu

13 months ago

Toggle Indicator on Widget

#1 @ocean90
12 months ago

  • Component changed from General to Widgets
  • Focuses accessibility added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#2 @welcher
12 months ago

  • Keywords good-first-bug added

#3 @RubenBristian
12 months ago

I confirm this, will try a patch as my first ticket at contributor day.

Last edited 12 months ago by RubenBristian (previous) (diff)

12 months ago

#4 @xavortm
12 months ago

  • Keywords has-patch added; needs-patch removed

Attached a patch where the markup and styling from the "Edit post" .postbox was used to replicate the mentioned effect on :focus. Some duplicate of stylings is happening between common.js and widgets.css file with different selectors, but in order to keep the different type of elements in their own designated files seemed like the proper way to go.

Also the fix aligns the arrow to the arrows below it as pre-patch there was about 2-3px offset to the right compared to all arrows below it.

I tested for the Appearance->Widget page elements only for now.

That aside some opinion - I think the markup is needlessly different between visually similar elements. If we can reuse classes on most of the components this type of duplication won't be needed. Feedback?

Last edited 12 months ago by xavortm (previous) (diff)

#5 @mihai2u
12 months ago

Thank you @xavortm for your work.
I'll check out your diff locally and confirm the fix with a screenshot.

I agree that it's better to re-use classes on similar looking elements.

#6 @mihai2u
12 months ago

I'm making some improvement by dropping some prefixes which are only needed for less than 0.1% of the users:

Also I confirmed this to be working perfectly on the Widgets view, matching the toggle button focus states available on the page meta boxes. The thing I'm not seeing though is the same thing implemented for the Menus.

@xavortm I will work on adding the same on the Menus. All of this is an accessibility enhancement allowing better and easier to follow keyboard navigation.

The main toggler focus view:

The sub togglers focus view:

12 months ago

12 months ago

#7 @mihai2u
12 months ago

I've worked on top of xavortm's work and added the focus states on the arrows next to the accordions, like the ones available on the Menus.

The previous diff I uploaded handles _just_ the focus state, but from an accessibility point of view, this wasn't yet perfect. With this, more elaborate, update I've adjusted a bit more of the menu accordion to work in a much similar fashion when using keyboard navigation. In the previous implementation with an open accordion, if you focused the next with the keyboard you wouldn't know which of the two was the actual one with the focus, which might cause confusion.

This is how the menu looks now when navigating with the keyboard from one to the next in most browsers, or when clicking specifically the arrow in Chrome:

#8 @afercia
11 months ago

  • Version 4.5.2 deleted

Thanks everyone for the ticket and the patches :) Couple things about the patch so far:

Widgets: hovering "Available Widgets", an arrow appears misplaced:

Menus: the accordion titles are too big and sidebar_name is undefined:

About vendor prefixes and other things (e.g. use tabs and not spaces to indent code) please see the CSS coding standards. For some background about the "circular focus" see #33808 and previously #32395.

(side note: the version number is meant to indicate the version the bug/feature was first introduced, when it is possible to track that to a specific version)

#9 @mihai2u
2 months ago

I'm refreshing this ticket.

Based on the latest WordPress version:

  • Toggle indicator in pages/post still has visible focus color (blue border radius)
  • Toggle menu accordion now has the same background-color switch only visual treatment and no blue ring on the arrow
  • Toggle widgets is not accessible via the keyboard but has the ring focus treatment if someone clicks on the arrow

Therefore I'm updating the diff to work with the latest WP and investigate towards working on a fix for afercia's feedback with the hovering of the available widgets.

Last edited 2 months ago by mihai2u (previous) (diff)

#10 @mihai2u
2 months ago

The ticket was refreshed to the latest WP and the feedback received from @afercia has been handled.
The reason of the undefined variable has been found and fixed. The title menus font sizes has been brought back to normal. The mispositioned arrow on hover from available widgets has been fixed and I also worked on it benefiting from the same accessibility improvements like the rest of them.

PR on github:

And I'm uploading the https://patch-diff.githubusercontent.com/raw/xwp/wordpress-develop/pull/222.diff as 37013.4.diff in here.

I created a 2 minutes video demo showing the accessibility improvements done under both the menus and the widgets pages:
(I recorded the video before fixing the font sizing of the menu accordion headers, which I've done with adding back the hndle class to them)

2 months ago

#11 @mihai2u
2 months ago

  • Keywords has-screenshots added

2 months ago

#12 @mihai2u
2 months ago

  • Keywords needs-testing added

#13 @lukecavanagh
2 months ago


37013.5.diff Applies cleanly and seems to work well.

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

2 months ago

#15 @westonruter
2 months ago

@afercia how's it look for you?

#16 @afercia
2 months ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.8
  • Owner set to afercia
  • Status changed from new to assigned

@westonruter there's a previous pending patch on #31476 that's ready for commit and the latest patch here is going to conflict with that.
Apart from this, it's a bit hard to follow all the changes here. Changing color alone was an easy good first bug :) Changing element types, JS, a11y, CSS position and display, and many other details it's something that goes a bit beyond the original scope of the ticket. That's not to say it shouldn't be done, just to say it will require me a bit of time to check everything. Overall, looks like a nice improvement, thanks to everyone! One minor detail so far: buttons don't need a keydown event.

I'd rather commit #31476 first and then review the changes here, if no objections.

#17 @Kopepasah
2 months ago

Ran a few checks and all changes by @mihai2u are working as expected. Just to further expound on on @afercia's comment about the keydown event, modern browsers recognize the action of space and return keys as a click event (from my understanding).

@afercia if you see potential conflicts, I would say commit #31476 first, other than what you mentioned, it looks good to me.

Last edited 2 months ago by Kopepasah (previous) (diff)

#18 follow-up: @mihai2u
2 months ago


Thank you for the reference. I see that the #31476 ticket is now merged in, therefore I will refresh my work to avoid any conflicts and update this thread with a new patch.
Also refering to this comment on that ticket:
"There's room for further enhancements, they should go in separate tickets. For example, other elements in the widgets screen are still not fully operable with a keyboard."
I'm covering exactly that in this update.
The work there complements nicely with what I did here, as it covers a different set of toggles and enables proper keyboard navigation.

Beyond the video walkthrough uploaded above, I can post a compact roadmap of the code updates, such that it's easy to follow from a development perspective as well.

#19 @mihai2u
2 months ago

  • Keywords needs-refresh added

#20 in reply to: ↑ 18 @afercia
2 months ago

Replying to mihai2u:

Also refering to this comment on that ticket:
"There's room for further enhancements, they should go in separate tickets. For example, other elements in the widgets screen are still not fully operable with a keyboard."
I'm covering exactly that in this update.
The work there complements nicely with what I did here

Yep, and it's a very welcomed improvement 🙂

#21 @mihai2u
7 weeks ago

  • Keywords needs-refresh removed

I've refreshed the patch with the latest in master. There were 2 small conflicts on common.css which I've handled easily, and created a quick video showing the widgets page now navigating through both the widget areas and the widgets themselves (as part of the improvements in #31476) and they play nice together.

The menus still work like in the previous video I uploaded, so we're also good on that front.

Now to make it easier for you to go through this, I'll go through the code changes to explain what I did.

Accessibility improvement
In these two commits targeting the Widgets page (HTML change):
I replaced the div which was only used as a graphic representation of the state of the box with a button, because we want to be able to interact with it.

In this commit targeting the Menu accordion (HTML change):
I removed the tabindex from the H3 element, and deleted the span that was showing the arrow state.
Instead I added inside the H3 element the button, because it's a better user experience to be able to highlight the arrow instead of the entire heading when focusing with the keyboard. This way it doesn't get confused with the currently open accordion heading.
In the following related commit I added back the hndle class, which was needed for styling purposes:

In this commit targeting the Menu accordion (JS change):
I added the .handlediv (the buttons I added used this class) to the JS selector in order to allow it to handle the expanding / closing of the accordions.
To the comment regarding buttons not needing the keydown event, I am aware this isn't necessary for the current modern browsers, but since the code was already there, I thought it might be targeting some IE relative, and didn't want to introduce a potential bug, going by the don't fix it if it ain't broken philosophy.

In this commit all of the styling changes are pulled together:
I'll go through the changes here in no particular order:

  • with the introduction of the .handlediv I styled the arrow inside it (closed / open states)
  • moved the content: "\f142" from the collapse arrow indicator in a separate set of selectors in order to allow it to be easily reused on other places where it might be needed. It's a tiny code refactoring which makes things more readable with the icon having the open state / closed state comments before it.
  • the old sidebar arrow had been replaced, some selectors/styles which used to be connected to it got removed
  • the metabox-holder styles section represents the new styles connected to the arrow button in the menu
  • the sidebar / widgets .handlediv styles section represents the styles connected to the arrow button and its focus state on the widgets page

Hope it will be easy to follow along the code with my indications and links to individual git commits.

I'll upload the refreshed patch next.

7 weeks ago

7 weeks ago

#22 @mihai2u
7 weeks ago

Updated patch with the merge conflict solved is attached (37013.7.diff) matching the

Looking forward to your feedback or core commit @afercia

Thank you!

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

7 weeks ago

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

6 weeks ago

#25 @afercia
6 weeks ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

#26 @westonruter
2 weeks ago

  • Milestone changed from 4.8.1 to 4.9
Note: See TracTickets for help on using tickets.